diff --git a/app/components/admin/stats/budget_balloting_component.rb b/app/components/admin/stats/budget_balloting_component.rb index 9b9065c44..12addf185 100644 --- a/app/components/admin/stats/budget_balloting_component.rb +++ b/app/components/admin/stats/budget_balloting_component.rb @@ -8,7 +8,7 @@ class Admin::Stats::BudgetBallotingComponent < ApplicationComponent private def stats - @stats ||= Budget::Stats.new(budget) + @stats ||= Budget::Stats.new(budget, cache: false) end def headings_stats diff --git a/app/components/admin/stats/budget_supporting_component.rb b/app/components/admin/stats/budget_supporting_component.rb index 5d7b9cea2..0fdeb7b20 100644 --- a/app/components/admin/stats/budget_supporting_component.rb +++ b/app/components/admin/stats/budget_supporting_component.rb @@ -8,7 +8,7 @@ class Admin::Stats::BudgetSupportingComponent < ApplicationComponent private def stats - @stats ||= Budget::Stats.new(budget) + @stats ||= Budget::Stats.new(budget, cache: false) end def headings_stats diff --git a/app/controllers/budgets/stats_controller.rb b/app/controllers/budgets/stats_controller.rb index 33480238d..353ff113b 100644 --- a/app/controllers/budgets/stats_controller.rb +++ b/app/controllers/budgets/stats_controller.rb @@ -8,7 +8,7 @@ module Budgets def show authorize! :read_stats, @budget - @stats = Budget::Stats.new(@budget) + @stats = Budget::Stats.new(@budget).tap(&:generate) @headings = @budget.headings.sort_by_name end diff --git a/app/controllers/polls_controller.rb b/app/controllers/polls_controller.rb index ecb8ffadb..80b72309b 100644 --- a/app/controllers/polls_controller.rb +++ b/app/controllers/polls_controller.rb @@ -23,7 +23,7 @@ class PollsController < ApplicationController end def stats - @stats = Poll::Stats.new(@poll) + @stats = Poll::Stats.new(@poll).tap(&:generate) end def results diff --git a/app/lib/geozone_stats.rb b/app/lib/geozone_stats.rb deleted file mode 100644 index 6b7971610..000000000 --- a/app/lib/geozone_stats.rb +++ /dev/null @@ -1,24 +0,0 @@ -class GeozoneStats - attr_reader :geozone, :participants - - def initialize(geozone, participants) - @geozone = geozone - @participants = participants - end - - def geozone_participants - participants.where(geozone: geozone) - end - - def name - geozone.name - end - - def count - geozone_participants.count - end - - def percentage - PercentageCalculator.calculate(count, participants.count) - end -end diff --git a/app/lib/percentage_calculator.rb b/app/lib/percentage_calculator.rb deleted file mode 100644 index bb8f3d355..000000000 --- a/app/lib/percentage_calculator.rb +++ /dev/null @@ -1,7 +0,0 @@ -module PercentageCalculator - def self.calculate(fraction, total) - return 0.0 if total.zero? - - (fraction * 100.0 / total).round(3) - end -end diff --git a/app/models/budget.rb b/app/models/budget.rb index 4fc250bf4..319f02d4b 100644 --- a/app/models/budget.rb +++ b/app/models/budget.rb @@ -1,7 +1,6 @@ class Budget < ApplicationRecord include Measurable include Sluggable - include StatsVersionable include Reportable include Imageable diff --git a/app/models/budget/stats.rb b/app/models/budget/stats.rb index 4d62cd6de..d03e4edd3 100644 --- a/app/models/budget/stats.rb +++ b/app/models/budget/stats.rb @@ -192,7 +192,7 @@ class Budget::Stats stats_cache(*stats_methods) - def stats_cache(key, &) - Rails.cache.fetch("budgets_stats/#{budget.id}/#{phases.join}/#{key}/#{version}", &) + def full_cache_key_for(key) + "budgets_stats/#{budget.id}/#{phases.join}/#{key}" end end diff --git a/app/models/concerns/statisticable.rb b/app/models/concerns/statisticable.rb index 53533b4ed..f8b13766a 100644 --- a/app/models/concerns/statisticable.rb +++ b/app/models/concerns/statisticable.rb @@ -3,7 +3,7 @@ module Statisticable PARTICIPATIONS = %w[gender age geozone].freeze included do - attr_reader :resource + attr_reader :resource, :cache end class_methods do @@ -42,12 +42,28 @@ module Statisticable end end - def initialize(resource) + def initialize(resource, cache: true) @resource = resource + @cache = cache end def generate - stats_methods.each { |stat_name| send(stat_name) } + User.transaction do + begin + define_singleton_method :participants do + create_participants_table unless participants_table_created? + participants_class.all + end + + stats_methods.each { |stat_name| send(stat_name) } + ensure + define_singleton_method :participants do + participants_from_original_table + end + end + + drop_participants_table + end end def stats_methods @@ -77,6 +93,7 @@ module Statisticable def participants @participants ||= User.unscoped.where(id: participant_ids) end + alias_method :participants_from_original_table, :participants def total_male_participants participants.male.count @@ -114,23 +131,23 @@ module Statisticable end def participants_by_geozone - geozone_stats.to_h do |stats| + geozones.to_h do |geozone| + count = participants.where(geozone: geozone).count + [ - stats.name, + geozone.name, { - count: stats.count, - percentage: stats.percentage + count: count, + percentage: calculate_percentage(count, total_participants) } ] end end def calculate_percentage(fraction, total) - PercentageCalculator.calculate(fraction, total) - end + return 0.0 if total.zero? - def version - "v#{resource.find_or_create_stats_version.updated_at.to_i}" + (fraction * 100.0 / total).round(3) end def advanced? @@ -147,6 +164,34 @@ module Statisticable participations.map { |participation| self.class.send("#{participation}_methods") }.flatten end + def create_participants_table + User.connection.create_table( + participants_table_name, + temporary: true, + as: participants_from_original_table.to_sql + ) + User.connection.add_index participants_table_name, :date_of_birth + User.connection.add_index participants_table_name, :geozone_id + @participants_table_created = true + end + + def drop_participants_table + User.connection.drop_table(participants_table_name, if_exists: true, temporary: true) + @participants_table_created = false + end + + def participants_table_name + @participants_table_name ||= "participants_#{resource.class.table_name}_#{resource.id}" + end + + def participants_class + @participants_class ||= Class.new(User).tap { |klass| klass.table_name = participants_table_name } + end + + def participants_table_created? + @participants_table_created.present? + end + def total_participants_with_gender @total_participants_with_gender ||= participants.where.not(gender: nil).distinct.count end @@ -170,18 +215,10 @@ module Statisticable [90, 300]] end - def participants_between_ages(from, to) - participants.between_ages(from, to) - end - def geozones Geozone.order("name") end - def geozone_stats - geozones.map { |geozone| GeozoneStats.new(geozone, participants) } - end - def range_description(start, finish) if finish > 200 I18n.t("stats.age_more_than", start: start) @@ -189,4 +226,12 @@ module Statisticable I18n.t("stats.age_range", start: start, finish: finish) end end + + def stats_cache(key, &block) + if cache + Rails.cache.fetch(full_cache_key_for(key), expires_at: Date.current.end_of_day, &block) + else + block.call + end + end end diff --git a/app/models/concerns/stats_versionable.rb b/app/models/concerns/stats_versionable.rb deleted file mode 100644 index 9e9138ecc..000000000 --- a/app/models/concerns/stats_versionable.rb +++ /dev/null @@ -1,11 +0,0 @@ -module StatsVersionable - extend ActiveSupport::Concern - - included do - has_one :stats_version, as: :process, inverse_of: :process - end - - def find_or_create_stats_version - stats_version || create_stats_version - end -end diff --git a/app/models/poll.rb b/app/models/poll.rb index 3d587b59f..4021fcf1c 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -7,7 +7,6 @@ class Poll < ApplicationRecord include Notifiable include Searchable include Sluggable - include StatsVersionable include Reportable include SDG::Relatable diff --git a/app/models/poll/stats.rb b/app/models/poll/stats.rb index c449bf17d..8a0b1562b 100644 --- a/app/models/poll/stats.rb +++ b/app/models/poll/stats.rb @@ -121,7 +121,7 @@ class Poll::Stats stats_cache(*stats_methods) - def stats_cache(key, &) - Rails.cache.fetch("polls_stats/#{poll.id}/#{key}/#{version}", &) + def full_cache_key_for(key) + "polls_stats/#{poll.id}/#{key}" end end diff --git a/app/models/stats_version.rb b/app/models/stats_version.rb deleted file mode 100644 index f47ddea44..000000000 --- a/app/models/stats_version.rb +++ /dev/null @@ -1,5 +0,0 @@ -class StatsVersion < ApplicationRecord - validates :process, presence: true - - belongs_to :process, polymorphic: true -end diff --git a/config/schedule.rb b/config/schedule.rb index b0755dbaf..fcfd626e3 100644 --- a/config/schedule.rb +++ b/config/schedule.rb @@ -27,10 +27,6 @@ every 1.day, at: "5:00 am" do rake "-s sitemap:refresh:no_ping" end -every 2.hours do - rake "-s stats:generate" -end - every 1.day, at: "1:00 am", roles: [:cron] do rake "files:remove_old_cached_attachments" end diff --git a/db/migrate/20240408154814_drop_stats_versions.rb b/db/migrate/20240408154814_drop_stats_versions.rb new file mode 100644 index 000000000..a88c1d879 --- /dev/null +++ b/db/migrate/20240408154814_drop_stats_versions.rb @@ -0,0 +1,12 @@ +class DropStatsVersions < ActiveRecord::Migration[7.0] + def change + drop_table :stats_versions, id: :serial do |t| + t.string :process_type + t.integer :process_id + t.datetime :created_at, precision: nil, null: false + t.datetime :updated_at, precision: nil, null: false + + t.index ["process_type", "process_id"], name: "index_stats_versions_on_process_type_and_process_id" + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 0b5dfa0d5..d330136cc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1515,14 +1515,6 @@ ActiveRecord::Schema[7.0].define(version: 2024_04_24_013913) do t.string "locale" end - create_table "stats_versions", id: :serial, force: :cascade do |t| - t.string "process_type" - t.integer "process_id" - t.datetime "created_at", precision: nil, null: false - t.datetime "updated_at", precision: nil, null: false - t.index ["process_type", "process_id"], name: "index_stats_versions_on_process_type_and_process_id" - end - create_table "taggings", id: :serial, force: :cascade do |t| t.integer "tag_id" t.string "taggable_type" diff --git a/lib/tasks/stats.rake b/lib/tasks/stats.rake deleted file mode 100644 index b7d397301..000000000 --- a/lib/tasks/stats.rake +++ /dev/null @@ -1,32 +0,0 @@ -namespace :stats do - desc "Generates stats which are not cached yet" - task generate: :environment do - ApplicationLogger.new.info "Updating budget and poll stats" - - Tenant.run_on_each do - admin_ability = Ability.new(Administrator.first.user) - - Budget.accessible_by(admin_ability, :read_stats).find_each do |budget| - Budget::Stats.new(budget).generate - print "." - end - - Poll.accessible_by(admin_ability, :stats).find_each do |poll| - Poll::Stats.new(poll).generate - print "." - end - end - end - - desc "Expires stats cache" - task expire_cache: :environment do - Tenant.run_on_each do - [Budget, Poll].each do |model_class| - model_class.find_each { |record| record.find_or_create_stats_version.touch } - end - end - end - - desc "Deletes stats cache and generates it again" - task regenerate: [:expire_cache, :generate] -end diff --git a/spec/lib/geozone_stats_spec.rb b/spec/lib/geozone_stats_spec.rb deleted file mode 100644 index 570315406..000000000 --- a/spec/lib/geozone_stats_spec.rb +++ /dev/null @@ -1,44 +0,0 @@ -require "rails_helper" - -describe GeozoneStats do - let(:winterfell) { create(:geozone, name: "Winterfell") } - let(:riverlands) { create(:geozone, name: "Riverlands") } - - describe "#name" do - let(:stats) { GeozoneStats.new(winterfell, []) } - - it "returns the geozone name" do - expect(stats.name).to eq "Winterfell" - end - end - - describe "#count" do - before do - 2.times { create(:user, geozone: winterfell) } - 1.times { create(:user, geozone: riverlands) } - end - - let(:winterfell_stats) { GeozoneStats.new(winterfell, User.all) } - let(:riverlands_stats) { GeozoneStats.new(riverlands, User.all) } - - it "counts participants from the geozone" do - expect(winterfell_stats.count).to eq 2 - expect(riverlands_stats.count).to eq 1 - end - end - - describe "#percentage" do - before do - 2.times { create(:user, geozone: winterfell) } - 1.times { create(:user, geozone: riverlands) } - end - - let(:winterfell_stats) { GeozoneStats.new(winterfell, User.all) } - let(:riverlands_stats) { GeozoneStats.new(riverlands, User.all) } - - it "calculates percentage relative to the amount of participants" do - expect(winterfell_stats.percentage).to eq 66.667 - expect(riverlands_stats.percentage).to eq 33.333 - end - end -end diff --git a/spec/models/poll/stats_spec.rb b/spec/models/poll/stats_spec.rb index ddb95a361..9bf5f56c2 100644 --- a/spec/models/poll/stats_spec.rb +++ b/spec/models/poll/stats_spec.rb @@ -299,32 +299,4 @@ describe Poll::Stats do end end end - - describe "#version", :with_frozen_time do - context "record with no stats" do - it "returns a string based on the current time" do - expect(stats.version).to eq "v#{Time.current.to_i}" - end - - it "doesn't overwrite the timestamp when called multiple times" do - time = Time.current - - expect(stats.version).to eq "v#{time.to_i}" - - unfreeze_time - - travel_to 2.seconds.from_now do - expect(stats.version).to eq "v#{time.to_i}" - end - end - end - - context "record with stats" do - before { poll.create_stats_version(updated_at: 1.day.ago) } - - it "returns the version of the existing stats" do - expect(stats.version).to eq "v#{1.day.ago.to_i}" - end - end - end end diff --git a/spec/models/statisticable_spec.rb b/spec/models/statisticable_spec.rb index 9768b39ee..fa3301610 100644 --- a/spec/models/statisticable_spec.rb +++ b/spec/models/statisticable_spec.rb @@ -4,10 +4,21 @@ describe Statisticable do before do dummy_stats = Class.new do include Statisticable + attr_accessor :total + stats_cache :total + + def full_cache_key_for(key) + "dummy_stats/#{object_id}/#{key}" + end def participants User.all end + alias_method :participants_from_original_table, :participants + + def total_participants + User.count + end def participation_date Time.current @@ -17,7 +28,8 @@ describe Statisticable do stub_const("DummyStats", dummy_stats) end - let(:stats) { DummyStats.new(nil) } + let(:resource) { double(id: 1, class: double(table_name: "")) } + let(:stats) { DummyStats.new(resource) } describe "#gender?" do context "No participants" do @@ -194,4 +206,65 @@ describe Statisticable do end end end + + describe "#generate" do + it "drops the temporary table after finishing" do + stats.generate + + expect { stats.send(:participants_class).first }.to raise_exception(ActiveRecord::StatementInvalid) + end + + it "can be executed twice without errors" do + stats.generate + + expect { stats.generate }.not_to raise_exception + end + + it "can be executed twice in parallel since it uses a transaction" do + other_stats = DummyStats.new(stats.resource) + + [stats, other_stats].map do |stat| + Thread.new { stat.generate } + end.each(&:join) + end + end + + describe "cache", :with_cache do + it "expires the cache at the end of the day by default" do + time = Time.current + + travel_to(time) do + stats.total = 6 + + expect(stats.total).to eq 6 + + stats.total = 7 + + expect(stats.total).to eq 6 + end + + travel_to(time.end_of_day) do + expect(stats.total).to eq 6 + end + + travel_to(time.end_of_day + 1.second) do + expect(stats.total).to eq 7 + end + end + + it "does not use the cache with cache: false" do + stats = DummyStats.new(resource, cache: false) + time = Time.current + + travel_to(time) do + stats.total = 6 + + expect(stats.total).to eq 6 + + stats.total = 7 + + expect(stats.total).to eq 7 + end + end + end end diff --git a/spec/models/stats_version_spec.rb b/spec/models/stats_version_spec.rb deleted file mode 100644 index eb875ed09..000000000 --- a/spec/models/stats_version_spec.rb +++ /dev/null @@ -1,13 +0,0 @@ -require "rails_helper" - -describe StatsVersion do - describe "validations" do - it "is valid with a process" do - expect(StatsVersion.new(process: Budget.new)).to be_valid - end - - it "is not valid without a process" do - expect(StatsVersion.new(process: nil)).not_to be_valid - end - end -end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a82d425e6..15c49772e 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -193,6 +193,11 @@ RSpec.configure do |config| savon.unmock! end + config.before(:each, :with_cache) do + allow(Rails).to receive(:cache).and_return(ActiveSupport::Cache.lookup_store(:memory_store)) + Rails.cache.clear + end + # Allows RSpec to persist some state between runs in order to support # the `--only-failures` and `--next-failure` CLI options. config.example_status_persistence_file_path = "spec/examples.txt" diff --git a/spec/system/admin/stats_spec.rb b/spec/system/admin/stats_spec.rb index 88c77cf2d..2ea281266 100644 --- a/spec/system/admin/stats_spec.rb +++ b/spec/system/admin/stats_spec.rb @@ -99,6 +99,33 @@ describe "Stats", :admin do expect(page).to have_link "Go back", count: 1 end + scenario "Don't use the cache for supports", :with_cache do + budget.update!(phase: :selecting) + investment = create(:budget_investment, heading: heading_all_city) + create(:user, :level_two, votables: [investment]) + + supporter = create(:user, :level_two) + + visit budget_supporting_admin_stats_path(budget_id: budget) + + expect(page).to have_content "VOTES\n1" + expect(page).to have_content "PARTICIPANTS\n1" + + in_browser(:supporter) do + login_as(supporter) + + visit budget_investment_path(budget, investment) + click_button "Support" + + expect(page).to have_button "Remove your support" + end + + refresh + + expect(page).to have_content "VOTES\n2" + expect(page).to have_content "PARTICIPANTS\n2" + end + scenario "hide final voting link" do visit admin_stats_path click_link "Participatory Budgets" @@ -135,6 +162,32 @@ describe "Stats", :admin do expect(page).to have_content "VOTES\n3" expect(page).to have_content "PARTICIPANTS\n2" end + + scenario "Don't use the cache for votes", :with_cache do + budget.update!(phase: :balloting) + create(:user, ballot_lines: [investment]) + + balloter = create(:user, :level_two) + + visit budget_balloting_admin_stats_path(budget_id: budget.id) + + expect(page).to have_content "VOTES\n1" + expect(page).to have_content "PARTICIPANTS\n1" + + in_browser(:balloter) do + login_as(balloter) + + visit budget_investment_path(budget, investment) + click_button "Vote" + + expect(page).to have_button "Remove vote" + end + + refresh + + expect(page).to have_content "VOTES\n2" + expect(page).to have_content "PARTICIPANTS\n2" + end end end