Don't use the cache in admin budget stats
In commite51e03446, 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 commite51e03446. Co-authored-by: Javi Martín <javim@elretirao.net>
This commit is contained in:
committed by
Javi Martín
parent
a4461a1a56
commit
16f844595d
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user