Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
8 changes: 8 additions & 0 deletions app/models/admin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 14 additions & 2 deletions app/models/concerns/admin_handler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -277,12 +277,24 @@ def model_data_type
# Invalidate the cache and latest update value
# @return [<Type>] <description>
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
8 changes: 8 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand Down
47 changes: 47 additions & 0 deletions spec/helpers/application_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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/<token>/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
72 changes: 72 additions & 0 deletions spec/models/concerns/admin_handler_cache_invalidation_spec.rb
Original file line number Diff line number Diff line change
@@ -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/<token>/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