Merge branch '336220-user-details-discussions-cache' into 'master'

Handle cache busting scenario when note author detail changes

See merge request gitlab-org/gitlab!66814

app/models/note.rb

@@ -587,7 +587,7 @@ class Note < ApplicationRecord
   end
 
   def post_processed_cache_key
-    cache_key_items = [cache_key]
+    cache_key_items = [cache_key, author.cache_key]
     cache_key_items << Digest::SHA1.hexdigest(redacted_note_html) if redacted_note_html.present?
 
     cache_key_items.join(':')

app/services/users/set_status_service.rb

@@ -28,11 +28,12 @@ module Users
       params[:emoji] = UserStatus::DEFAULT_EMOJI if params[:emoji].blank?
       params[:availability] = UserStatus.availabilities[:not_set] unless new_user_availability
 
-      user_status.update(params)
+      bump_user if user_status.update(params)
     end
 
     def remove_status
-      UserStatus.delete(target_user.id)
+      bump_user if UserStatus.delete(target_user.id).nonzero?
+      true
     end
 
     def user_status
@@ -48,5 +49,12 @@ module Users
     def new_user_availability
       UserStatus.availabilities[params[:availability]]
     end
+
+    def bump_user
+      # Intentionally not calling `touch` as that will trigger other callbacks
+      # on target_user (e.g. after_touch, after_commit, after_rollback) and we
+      # don't need them to happen here.
+      target_user.update_column(:updated_at, Time.current)
+    end
   end
 end

spec/models/note_spec.rb

@@ -1542,8 +1542,8 @@ RSpec.describe Note do
   describe '#post_processed_cache_key' do
     let(:note) { build(:note) }
 
-    it 'returns cache key by default' do
-      expect(note.post_processed_cache_key).to eq(note.cache_key)
+    it 'returns cache key and author cache key by default' do
+      expect(note.post_processed_cache_key).to eq("#{note.cache_key}:#{note.author.cache_key}")
     end
 
     context 'when note has redacted_note_html' do
@@ -1554,7 +1554,7 @@ RSpec.describe Note do
       end
 
       it 'returns cache key with redacted_note_html sha' do
-        expect(note.post_processed_cache_key).to eq("#{note.cache_key}:#{Digest::SHA1.hexdigest(redacted_note_html)}")
+        expect(note.post_processed_cache_key).to eq("#{note.cache_key}:#{note.author.cache_key}:#{Digest::SHA1.hexdigest(redacted_note_html)}")
       end
     end
   end

spec/requests/projects/merge_requests_discussions_spec.rb

@@ -55,7 +55,8 @@ RSpec.describe 'merge requests discussions' do
 
     context 'caching', :use_clean_rails_memory_store_caching do
       let(:reference) { create(:issue, project: project) }
-      let!(:first_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, note: "reference: #{reference.to_reference}") }
+      let(:author) { create(:user) }
+      let!(:first_note) { create(:diff_note_on_merge_request, author: author, noteable: merge_request, project: project, note: "reference: #{reference.to_reference}") }
       let!(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note, noteable: merge_request, project: project) }
       let!(:award_emoji) { create(:award_emoji, awardable: first_note) }
 
@@ -181,6 +182,26 @@ RSpec.describe 'merge requests discussions' do
         end
       end
 
+      context 'when author detail changes' do
+        before do
+          author.update!(name: "#{author.name} (Updated)")
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note] }
+        end
+      end
+
+      context 'when author status changes' do
+        before do
+          Users::SetStatusService.new(author, message: "updated status").execute
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note] }
+        end
+      end
+
       context 'when merge_request_discussion_cache is disabled' do
         before do
           stub_feature_flags(merge_request_discussion_cache: false)

spec/services/users/set_status_service_spec.rb

@@ -8,6 +8,18 @@ RSpec.describe Users::SetStatusService do
   subject(:service) { described_class.new(current_user, params) }
 
   describe '#execute' do
+    shared_examples_for 'bumps user' do
+      it 'bumps User#updated_at' do
+        expect { service.execute }.to change { current_user.updated_at }
+      end
+    end
+
+    shared_examples_for 'does not bump user' do
+      it 'does not bump User#updated_at' do
+        expect { service.execute }.not_to change { current_user.updated_at }
+      end
+    end
+
     context 'when params are set' do
       let(:params) { { emoji: 'taurus', message: 'a random status', availability: 'busy' } }
 
@@ -31,6 +43,8 @@ RSpec.describe Users::SetStatusService do
         expect(service.execute).to be(true)
       end
 
+      it_behaves_like 'bumps user'
+
       context 'when setting availability to not_set' do
         before do
           params[:availability] = 'not_set'
@@ -72,6 +86,8 @@ RSpec.describe Users::SetStatusService do
         it 'does not update the status if the current user is not allowed' do
           expect { service.execute }.not_to change { target_user.status }
         end
+
+        it_behaves_like 'does not bump user'
       end
     end
 
@@ -79,20 +95,28 @@ RSpec.describe Users::SetStatusService do
       let(:params) { {} }
 
       shared_examples 'removes user status record' do
-        it 'deletes the status' do
-          status = create(:user_status, user: current_user)
-
+        it 'deletes the user status record' do
           expect { service.execute }
-            .to change { current_user.reload.status }.from(status).to(nil)
+            .to change { current_user.reload.status }.from(user_status).to(nil)
         end
-      end
 
-      it_behaves_like 'removes user status record'
+        it_behaves_like 'bumps user'
+      end
 
-      context 'when not_set is given for availability' do
-        let(:params) { { availability: 'not_set' } }
+      context 'when user has existing user status record' do
+        let!(:user_status) { create(:user_status, user: current_user) }
 
         it_behaves_like 'removes user status record'
+
+        context 'when not_set is given for availability' do
+          let(:params) { { availability: 'not_set' } }
+
+          it_behaves_like 'removes user status record'
+        end
+      end
+
+      context 'when user has no existing user status record' do
+        it_behaves_like 'does not bump user'
       end
     end
   end