From 16f844595dd87be12f522857de8bcb66147c7a98 Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Sat, 30 Mar 2024 12:02:11 +0100 Subject: [PATCH] Don't use the cache in admin budget stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In commit e51e03446, we started using the same code to show stats in the public area and in the admin area. However, in doing so we introduced a bug, since stats in the public area are only shown after a certain part of the process has finished, meaning the stats appearing on the page never change (in theory), so it's perfectly fine to cache them. However, in the admin area stats can be accessed while the process is still ongoing, so caching the stats will lead to the wrong results being displayed. We've thought about expiring the cache when new supports or ballot lines are added; however, that means the methods calculating the stats for the supporting phase would expire when supports are added/removed but the methods calculating the stats for the voting phase would expire when ballot lines are added/removed. It gets even more complex because the `headings` method calculates stats for both the supporting and the voting phases. So, since loading stats in the admin section is fast even without the cache because they only load very basic statistics, we're taking the simple approach of disabling the cache in this case, so everything works the same way it did before commit e51e03446. Co-authored-by: Javi Martín --- .../admin/stats/budget_balloting_component.rb | 2 +- .../stats/budget_supporting_component.rb | 2 +- app/models/concerns/statisticable.rb | 13 +++-- spec/models/statisticable_spec.rb | 22 ++++++-- spec/system/admin/stats_spec.rb | 53 +++++++++++++++++++ 5 files changed, 83 insertions(+), 9 deletions(-) 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/models/concerns/statisticable.rb b/app/models/concerns/statisticable.rb index cb1fae680..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,8 +42,9 @@ module Statisticable end end - def initialize(resource) + def initialize(resource, cache: true) @resource = resource + @cache = cache end def generate @@ -226,7 +227,11 @@ module Statisticable end end - def stats_cache(key, &) - Rails.cache.fetch(full_cache_key_for(key), expires_at: Date.current.end_of_day, &) + 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/spec/models/statisticable_spec.rb b/spec/models/statisticable_spec.rb index e1705d86d..fa3301610 100644 --- a/spec/models/statisticable_spec.rb +++ b/spec/models/statisticable_spec.rb @@ -28,7 +28,8 @@ describe Statisticable do stub_const("DummyStats", dummy_stats) end - let(:stats) { DummyStats.new(double(id: 1, class: double(table_name: ""))) } + let(:resource) { double(id: 1, class: double(table_name: "")) } + let(:stats) { DummyStats.new(resource) } describe "#gender?" do context "No participants" do @@ -228,8 +229,8 @@ describe Statisticable do end end - describe "cache" do - it "expires the cache at the end of the day", :with_cache do + 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 @@ -250,5 +251,20 @@ describe Statisticable 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/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