Skip to content

Commit

Permalink
Generate stats on every request without caching them
Browse files Browse the repository at this point in the history
In commit e51e034, 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.

But, since now we're using a temporary table to calculate the stats,
generating them might take between 1 and 2 seconds even in processes
with a hundred thousand participants, meaning we can disable the cache
and generate the stats on every request instead. It might not scale so
well when there are millions of participants; we might re-enable the
cache or find a different way to optimize the stats calculations if we
ever get to that point.

Note that, since we're only using the temporary table inside the
`generate` method, we need to cache the results obtained inside this
method so they're available elsewhere. We're using instance variables
for this purpose. This means the code doesn't improve much with this
change, but at least we don't have to worry about when we should expire
the cache.

Since loading stats in the admin section is fast even without the cache
because they only load very basic statistics, we're not even generating
them first in this case, so everything works the same way it did before
commit e51e034.

Co-authored-by: Javi Martín <javim@elretirao.net>
  • Loading branch information
microweb10 and javierm committed Apr 18, 2024
1 parent d6b3f98 commit c277fb5
Show file tree
Hide file tree
Showing 13 changed files with 70 additions and 92 deletions.
1 change: 0 additions & 1 deletion app/models/budget.rb
@@ -1,7 +1,6 @@
class Budget < ApplicationRecord
include Measurable
include Sluggable
include StatsVersionable
include Reportable
include Imageable

Expand Down
4 changes: 0 additions & 4 deletions app/models/budget/stats.rb
Expand Up @@ -179,8 +179,4 @@ def supports(supportable)
end

stats_cache(*stats_methods)

def full_cache_key_for(key)
"budgets_stats/#{budget.id}/#{phases.join}/#{key}/#{version}"
end
end
14 changes: 8 additions & 6 deletions app/models/concerns/statisticable.rb
Expand Up @@ -145,10 +145,6 @@ def calculate_percentage(fraction, total)
(fraction * 100.0 / total).round(3)
end

def version
"v#{resource.find_or_create_stats_version.updated_at.to_i}"
end

def advanced?
resource.advanced_stats_enabled?
end
Expand Down Expand Up @@ -226,7 +222,13 @@ def range_description(start, finish)
end
end

def stats_cache(key, &)
Rails.cache.fetch(full_cache_key_for(key), &)
def stats_cache(key, &block)
instance_variable_name = key.to_s.gsub("?", "_question")

unless instance_variable_get("@#{instance_variable_name}")
instance_variable_set("@#{instance_variable_name}", block.call)
end

instance_variable_get("@#{instance_variable_name}")
end
end
11 changes: 0 additions & 11 deletions app/models/concerns/stats_versionable.rb

This file was deleted.

1 change: 0 additions & 1 deletion app/models/poll.rb
Expand Up @@ -7,7 +7,6 @@ class Poll < ApplicationRecord
include Notifiable
include Searchable
include Sluggable
include StatsVersionable
include Reportable
include SDG::Relatable

Expand Down
4 changes: 0 additions & 4 deletions app/models/poll/stats.rb
Expand Up @@ -116,8 +116,4 @@ def total_unregistered_booth
end

stats_cache(*stats_methods)

def full_cache_key_for(key)
"polls_stats/#{poll.id}/#{key}/#{version}"
end
end
5 changes: 0 additions & 5 deletions app/models/stats_version.rb

This file was deleted.

8 changes: 8 additions & 0 deletions db/migrate/20240408154814_drop_stats_versions.rb
@@ -0,0 +1,8 @@
class DropStatsVersions < ActiveRecord::Migration[6.1]
def change
drop_table :stats_versions do |t|
t.references :process, polymorphic: true, index: true
t.timestamps null: false
end
end
end
10 changes: 1 addition & 9 deletions db/schema.rb
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2024_03_22_223950) do
ActiveRecord::Schema[7.0].define(version: 2024_04_08_154814) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_trgm"
enable_extension "plpgsql"
Expand Down Expand Up @@ -1516,14 +1516,6 @@
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"
Expand Down
10 changes: 0 additions & 10 deletions lib/tasks/stats.rake

This file was deleted.

28 changes: 0 additions & 28 deletions spec/models/poll/stats_spec.rb
Expand Up @@ -259,32 +259,4 @@
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
13 changes: 0 additions & 13 deletions spec/models/stats_version_spec.rb

This file was deleted.

53 changes: 53 additions & 0 deletions spec/system/admin/stats_spec.rb
Expand Up @@ -111,6 +111,33 @@
expect(page).to have_link "Go back", count: 1
end

scenario "Clear cache after new 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"
Expand Down Expand Up @@ -147,6 +174,32 @@
expect(page).to have_content "VOTES\n3"
expect(page).to have_content "PARTICIPANTS\n2"
end

scenario "Clear cache after new 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

Expand Down

0 comments on commit c277fb5

Please sign in to comment.