Handle cache busting scenario when note author detail changes

Before when a note author which is part of a discussion has its
detail updated (e.g. name change, username change), the discussion
cache wont be busted.

To ensure that we show updated information when that happens, the
author's cache key is now included in
`Note#post_processed_cache_key` which we call in
`Discussion#cache_key`.

This way, when the author is updated, the discussion cache will be
invalidated as well.

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