From a4461a1a56332c3e1d97eb3310dc635ec29f8a78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 18 Apr 2024 22:16:26 +0200 Subject: [PATCH] Expire the stats cache once per day When we first started caching the stats, generating them was a process that took several minutes, so we never expired the cache. However, there have been cases where we run into issues where the stats shown on the screen were outdated. That's why we introduced a task to manually expire the cache. But now, generating the stats only takes a few seconds, so we can automatically expire them every day, remove all the logic needed to manually expire them, and get rid of most of the issues related to the cache being outdated. We're expiring them every day because it's the same day we were doing in public stats (which we removed in commit 631b48f58), only we're using `expires_at:` to set the expiration time, in order to simplify the code. Note that, in the test, we're using `travel_to(time)` so the test passes even when it starts an instant before midnight. We aren't using `:with_frozen_time` because, in similar cases (although not in this case, but I'm not sure whether that's intentional), `travel_to` shows this error: > Calling `travel_to` with a block, when we have previously already made > a call to `travel_to`, can lead to confusing time stubbing. --- app/models/budget.rb | 1 - app/models/budget/stats.rb | 2 +- app/models/concerns/statisticable.rb | 6 +--- app/models/concerns/stats_versionable.rb | 11 ------- app/models/poll.rb | 1 - app/models/poll/stats.rb | 2 +- app/models/stats_version.rb | 5 ---- .../20240408154814_drop_stats_versions.rb | 12 ++++++++ db/schema.rb | 8 ----- lib/tasks/stats.rake | 10 ------- spec/models/poll/stats_spec.rb | 28 ----------------- spec/models/statisticable_spec.rb | 30 +++++++++++++++++++ spec/models/stats_version_spec.rb | 13 -------- 13 files changed, 45 insertions(+), 84 deletions(-) delete mode 100644 app/models/concerns/stats_versionable.rb delete mode 100644 app/models/stats_version.rb create mode 100644 db/migrate/20240408154814_drop_stats_versions.rb delete mode 100644 lib/tasks/stats.rake delete mode 100644 spec/models/stats_version_spec.rb 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 04cc6d480..d03e4edd3 100644 --- a/app/models/budget/stats.rb +++ b/app/models/budget/stats.rb @@ -193,6 +193,6 @@ class Budget::Stats stats_cache(*stats_methods) def full_cache_key_for(key) - "budgets_stats/#{budget.id}/#{phases.join}/#{key}/#{version}" + "budgets_stats/#{budget.id}/#{phases.join}/#{key}" end end diff --git a/app/models/concerns/statisticable.rb b/app/models/concerns/statisticable.rb index d644f0dec..cb1fae680 100644 --- a/app/models/concerns/statisticable.rb +++ b/app/models/concerns/statisticable.rb @@ -149,10 +149,6 @@ module Statisticable (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 @@ -231,6 +227,6 @@ module Statisticable end def stats_cache(key, &) - Rails.cache.fetch(full_cache_key_for(key), &) + Rails.cache.fetch(full_cache_key_for(key), expires_at: Date.current.end_of_day, &) 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 0aeb93109..8a0b1562b 100644 --- a/app/models/poll/stats.rb +++ b/app/models/poll/stats.rb @@ -122,6 +122,6 @@ class Poll::Stats stats_cache(*stats_methods) def full_cache_key_for(key) - "polls_stats/#{poll.id}/#{key}/#{version}" + "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/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 526309823..000000000 --- a/lib/tasks/stats.rake +++ /dev/null @@ -1,10 +0,0 @@ -namespace :stats do - 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 -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 f1817fbbd..e1705d86d 100644 --- a/spec/models/statisticable_spec.rb +++ b/spec/models/statisticable_spec.rb @@ -4,6 +4,12 @@ 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 @@ -221,4 +227,28 @@ describe Statisticable do end.each(&:join) end end + + describe "cache" do + it "expires the cache at the end of the day", :with_cache 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 + 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