Devise can assign trackable fields, but only allow writes once/hourtc-cache-trackable-attributes

Not assigning the trackable fields seems to cause strange side-effects.

3 files changed, 18 insertions, 7 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 05f636c020a..0b358fd4b19 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -40,14 +40,15 @@ class User < ActiveRecord::Base
   devise :lockable, :recoverable, :rememberable, :trackable,
     :validatable, :omniauthable, :confirmable, :registerable
 
-  # Limit trackable fields to update at most once every hour
-  alias_method :devise_update_tracked_fields!, :update_tracked_fields!
-
+  # Override Devise::Models::Trackable#update_tracked_fields!
+  # to limit database writes to at most once every hour
   def update_tracked_fields!(request)
+    update_tracked_fields(request)
+
     lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i)
     return unless lease.try_obtain
 
-    devise_update_tracked_fields!(request)
+    save(validate: false)
   end
 
   attr_accessor :force_random_password
diff --git a/spec/features/groups/members/sorting_spec.rb b/spec/features/groups/members/sorting_spec.rb
index 608aedd3471..902d3f789ff 100644
--- a/spec/features/groups/members/sorting_spec.rb
+++ b/spec/features/groups/members/sorting_spec.rb
@@ -68,7 +68,7 @@ feature 'Groups > Members > Sorting', feature: true do
     expect(page).to have_css('.member-sort-dropdown .dropdown-toggle-text', text: 'Name, descending')
   end
 
-  scenario 'sorts by recent sign in' do
+  scenario 'sorts by recent sign in', :redis do
     visit_members_list(sort: :recent_sign_in)
 
     expect(first_member).to include(owner.name)
@@ -76,7 +76,7 @@ feature 'Groups > Members > Sorting', feature: true do
     expect(page).to have_css('.member-sort-dropdown .dropdown-toggle-text', text: 'Recent sign in')
   end
 
-  scenario 'sorts by oldest sign in' do
+  scenario 'sorts by oldest sign in', :redis do
     visit_members_list(sort: :oldest_sign_in)
 
     expect(first_member).to include(developer.name)
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 0b59916342e..c7ddd17872b 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -359,7 +359,17 @@ describe User, models: true do
 
       expect do
         user.update_tracked_fields!(request)
-      end.not_to change { user.current_sign_in_at }
+      end.not_to change { user.reload.current_sign_in_at }
+    end
+
+    it 'writes trackable attributes for a different user' do
+      user2 = create(:user)
+
+      user.update_tracked_fields!(request)
+
+      expect do
+        user2.update_tracked_fields!(request)
+      end.to change { user2.reload.current_sign_in_at }
     end
   end