From 7b6d74399cb247592dc879e0e604def7c0c9834d Mon Sep 17 00:00:00 2001 From: Phil Ayres Date: Thu, 16 Jul 2026 08:56:37 +0100 Subject: [PATCH] Fixed whole-cache clear on routine User/Admin saves - fixes #1270 --- app/helpers/application_helper.rb | 3 +- app/models/admin.rb | 8 +++ app/models/concerns/admin_handler.rb | 16 ++++- app/models/user.rb | 8 +++ spec/helpers/application_helper_spec.rb | 47 ++++++++++++ .../admin_handler_cache_invalidation_spec.rb | 72 +++++++++++++++++++ 6 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 spec/models/concerns/admin_handler_cache_invalidation_spec.rb diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 75f9661895..bf02ee0d16 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -310,7 +310,8 @@ def partial_cache_key(partial, force_user_or_admin: nil) end ver = Application.server_cache_version - res = "#{partial}-partial2-#{ver}-#{auth_type}-#{u&.id}-#{u&.current_sign_in_at}-#{u&.updated_at}-#{apptype}-#{@item_updates}-#{userrole}-#{uac}" + res = "#{partial}-partial2-#{ver}-#{auth_type}-#{u&.id}-#{u&.current_sign_in_at}-" \ + "#{apptype}-#{@item_updates}-#{userrole}-#{uac}" prev_key = "#{partial}-#{auth_type}-#{u&.id}" @@prev_partial_cache_key ||= {} prev = @@prev_partial_cache_key[prev_key] diff --git a/app/models/admin.rb b/app/models/admin.rb index 29f0c48c06..e00b3fb6e4 100644 --- a/app/models/admin.rb +++ b/app/models/admin.rb @@ -32,6 +32,14 @@ def timeout_in Settings::AdminTimeout end + # Devise trackable/lockable saves the admin record on every sign-in and failed + # login attempt. Only clear the whole Rails cache when the disabled flag + # actually changes, to avoid destroying the shared template/fragment cache + # (and server_cache_version) on routine sign-ins. + def clear_rails_cache_on_save? + saved_change_to_disabled? + end + # Standard Devise callback to allow accounts to be disabled or expired def active_for_authentication? otp_secret # prime the corruption flag so the check is self-contained diff --git a/app/models/concerns/admin_handler.rb b/app/models/concerns/admin_handler.rb index 45c2df9497..7500942e35 100644 --- a/app/models/concerns/admin_handler.rb +++ b/app/models/concerns/admin_handler.rb @@ -277,12 +277,24 @@ def model_data_type # Invalidate the cache and latest update value # @return [] def invalidate_cache - logger.info "Admin record added or updated (#{self.class.name}). Invalidating cache" - # Allows caching in other classes to reset self.class.reset_latest_update + return unless clear_rails_cache_on_save? + + logger.info "Admin record added or updated (#{self.class.name}). Invalidating cache" + # Unfortunately we have no way to clear pattern matched keys with memcached so we just clear the whole cache Rails.cache.clear end + + # + # Whether this save should clear the entire Rails cache. + # Defaults to true for all admin models. Override in models that are saved + # very frequently during normal operation (e.g. User, Admin) to limit + # the whole-cache clear to only the changes that actually require it. + # @return [Boolean] + def clear_rails_cache_on_save? + true + end end diff --git a/app/models/user.rb b/app/models/user.rb index aa08908bdd..f116ce10d0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -310,6 +310,14 @@ def set_app_type self.app_type_id = nil if app_type_id && !app_type_valid? end + # Devise trackable/lockable saves the user record on every sign-in and failed + # login attempt. Only clear the whole Rails cache when the disabled flag + # actually changes, to avoid destroying the shared template/fragment cache + # (and server_cache_version) on routine sign-ins. + def clear_rails_cache_on_save? + saved_change_to_disabled? + end + def password_required? return false if a_template_or_batch_user? diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index fe6173b40f..9107a72b62 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -9,6 +9,9 @@ # - Filters out DoNotDisplayErrorMessage markers while preserving valid error messages # - Removes entire error fields that contain only DoNotDisplayErrorMessage markers # - Ensures clean error display to users by eliminating internal marker constants +# - #partial_cache_key (issue #1270): the cache key/template version token stays stable +# across user saves that don't change relevant attributes, and still changes when the +# user's app type genuinely changes require 'rails_helper' @@ -122,3 +125,47 @@ end end end + +# Purpose (issue #1270): partial_cache_key previously embedded the user's +# `updated_at` timestamp. Since User is saved on almost every request (Devise +# trackable sign-in tracking, app type switching), this made the cache key - +# and therefore the /pages//template URL - change far more often than +# necessary, defeating the long-lived browser cache. These specs confirm the +# key stays stable across saves that don't affect its relevant inputs, and +# still changes when the app type genuinely changes. +describe '#partial_cache_key' do + include ModelSupport + + before :all do + create_admin + end + + before do + helper.define_singleton_method(:current_admin) { nil } + helper.define_singleton_method(:current_user) { @current_user } + end + + it 'is unchanged when the user is saved without a relevant attribute change' do + user, = create_user + helper.instance_variable_set(:@current_user, user) + + before_key = helper.partial_cache_key(:loaded, force_user_or_admin: user) + user.update!(first_name: 'Changed Name') + after_key = helper.partial_cache_key(:loaded, force_user_or_admin: user) + + expect(after_key).to eq(before_key) + end + + it 'changes when the user app type changes' do + user, = create_user + other_app_type = Admin::AppType.active.where.not(id: user.app_type_id).first + skip 'No second active app type available for this test' unless other_app_type + + before_key = helper.partial_cache_key(:loaded, force_user_or_admin: user) + user.current_admin = @admin + user.update!(app_type: other_app_type) + after_key = helper.partial_cache_key(:loaded, force_user_or_admin: user) + + expect(after_key).not_to eq(before_key) + end +end diff --git a/spec/models/concerns/admin_handler_cache_invalidation_spec.rb b/spec/models/concerns/admin_handler_cache_invalidation_spec.rb new file mode 100644 index 0000000000..fcc532f63d --- /dev/null +++ b/spec/models/concerns/admin_handler_cache_invalidation_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Purpose: demonstrate and verify the fix for issue #1270 - User and Admin +# records are saved on almost every request (Devise trackable sign-in +# tracking, lockable failed-attempt counters, app type switching), but +# AdminHandler#invalidate_cache previously called Rails.cache.clear on every +# save regardless of what changed. That wiped the shared template/fragment +# cache (and Application.server_cache_version) on routine sign-ins, causing +# browsers to refetch /pages//template within the same session even +# though no admin configuration had changed. +# +# These specs verify that: +# - User and Admin only trigger Rails.cache.clear when their `disabled` flag +# actually changes (via the new #clear_rails_cache_on_save? override). +# - Other AdminHandler-including models retain the original behaviour of +# clearing the cache on every save. +RSpec.describe AdminHandler, type: :model do + include ModelSupport + + before :example do + create_admin + end + + describe 'User' do + it 'does not clear the Rails cache when saved without a disabled change' do + user, = create_user + expect(Rails.cache).not_to receive(:clear) + user.update!(first_name: 'Changed') + end + + it 'does not clear the Rails cache on an app type change' do + user, = create_user + other_app_type = Admin::AppType.active.where.not(id: user.app_type_id).first + skip 'No second active app type available for this test' unless other_app_type + + expect(Rails.cache).not_to receive(:clear) + user.current_admin = @admin + user.update!(app_type: other_app_type) + end + + it 'clears the Rails cache when the disabled flag changes' do + user, = create_user + user.current_admin = @admin + expect(Rails.cache).to receive(:clear).at_least(:once) + user.update!(disabled: true) + end + end + + describe 'Admin' do + it 'does not clear the Rails cache when saved without a disabled change' do + admin, = UserSupport.create_admin + expect(Rails.cache).not_to receive(:clear) + admin.update!(first_name: 'Changed') + end + + it 'clears the Rails cache when the disabled flag changes' do + admin, = UserSupport.create_admin + expect(Rails.cache).to receive(:clear) + admin.update!(disabled: true) + end + end + + describe 'a standard AdminHandler model' do + it 'still clears the Rails cache on every save' do + expect(Rails.cache).to receive(:clear).at_least(:once) + Classification::GeneralSelection.create! item_type: 'player_contacts_type', name: 'Cache Test', + value: 'cache_test', current_admin: @admin + end + end +end