Merge branch '336204-mr-discussions-cache-state' into 'master'

Handle post processed content scenario for MR discussions cache

See merge request gitlab-org/gitlab!66354

app/models/discussion.rb

@@ -163,16 +163,15 @@ class Discussion
   end
 
   def cache_key
-    # Need this so cache will be invalidated when note within a discussion
-    # has been deleted.
-    notes_sha = Digest::SHA1.hexdigest(notes.map(&:id).join(':'))
+    # Need to use the notes' cache key so cache will be invalidated when note
+    # within a discussion has been deleted or has different data after post
+    # processing of content.
+    notes_sha = Digest::SHA1.hexdigest(notes.map(&:post_processed_cache_key).join(':'))
 
     [
       CACHE_VERSION,
-      notes.last.latest_cached_markdown_version,
       id,
       notes_sha,
-      notes.max_by(&:updated_at).updated_at,
       resolved_at
     ].join(':')
   end

app/models/note.rb

@@ -586,6 +586,13 @@ class Note < ApplicationRecord
     review.present? || !author.can_trigger_notifications?
   end
 
+  def post_processed_cache_key
+    cache_key_items = [cache_key]
+    cache_key_items << Digest::SHA1.hexdigest(redacted_note_html) if redacted_note_html.present?
+
+    cache_key_items.join(':')
+  end
+
   private
 
   # Using this method followed by a call to *save* may result in *ActiveRecord::RecordNotUnique* exception

spec/models/diff_discussion_spec.rb

@@ -129,10 +129,10 @@ RSpec.describe DiffDiscussion do
 
   describe '#cache_key' do
     it 'returns the cache key with the position sha' do
-      notes_sha = Digest::SHA1.hexdigest("#{diff_note.id}")
+      notes_sha = Digest::SHA1.hexdigest("#{diff_note.post_processed_cache_key}")
       position_sha = Digest::SHA1.hexdigest(diff_note.position.to_json)
 
-      expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{diff_note.latest_cached_markdown_version}:#{subject.id}:#{notes_sha}:#{diff_note.updated_at}::#{position_sha}")
+      expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{subject.id}:#{notes_sha}::#{position_sha}")
     end
   end
 end

spec/models/discussion_spec.rb

@@ -53,10 +53,10 @@ RSpec.describe Discussion do
   end
 
   describe '#cache_key' do
-    let(:notes_sha) { Digest::SHA1.hexdigest("#{first_note.id}:#{second_note.id}:#{third_note.id}") }
+    let(:notes_sha) { Digest::SHA1.hexdigest("#{first_note.post_processed_cache_key}:#{second_note.post_processed_cache_key}:#{third_note.post_processed_cache_key}") }
 
-    it 'returns the cache key with ID and latest updated note updated at' do
-      expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{third_note.latest_cached_markdown_version}:#{subject.id}:#{notes_sha}:#{third_note.updated_at}:")
+    it 'returns the cache key' do
+      expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{subject.id}:#{notes_sha}:")
     end
 
     context 'when discussion is resolved' do
@@ -65,7 +65,7 @@ RSpec.describe Discussion do
       end
 
       it 'returns the cache key with resolved at' do
-        expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{third_note.latest_cached_markdown_version}:#{subject.id}:#{notes_sha}:#{third_note.updated_at}:#{subject.resolved_at}")
+        expect(subject.cache_key).to eq("#{described_class::CACHE_VERSION}:#{subject.id}:#{notes_sha}:#{subject.resolved_at}")
       end
     end
   end

spec/models/note_spec.rb

@@ -1538,4 +1538,24 @@ RSpec.describe Note do
       expect(attachment).not_to be_exist
     end
   end
+
+  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)
+    end
+
+    context 'when note has redacted_note_html' do
+      let(:redacted_note_html) { 'redacted note html' }
+
+      before do
+        note.redacted_note_html = redacted_note_html
+      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)}")
+      end
+    end
+  end
 end

spec/requests/projects/merge_requests_discussions_spec.rb

@@ -54,7 +54,8 @@ RSpec.describe 'merge requests discussions' do
     end
 
     context 'caching', :use_clean_rails_memory_store_caching do
-      let!(:first_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
+      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!(: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) }
 
@@ -93,6 +94,16 @@ RSpec.describe 'merge requests discussions' do
         end
       end
 
+      context 'when a note in a discussion got its reference state updated' do
+        before do
+          reference.close!
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note] }
+        end
+      end
+
       context 'when a note in a discussion got resolved' do
         before do
           travel_to(1.minute.from_now) do
@@ -147,17 +158,6 @@ RSpec.describe 'merge requests discussions' do
         end
       end
 
-      context 'when cached markdown version gets bump' do
-        before do
-          settings = Gitlab::CurrentSettings.current_application_settings
-          settings.update!(local_markdown_version: settings.local_markdown_version + 1)
-        end
-
-        it_behaves_like 'cache miss' do
-          let(:changed_notes) { [first_note, second_note] }
-        end
-      end
-
       context 'when the diff note position changes' do
         before do
           # This replicates a position change wherein timestamps aren't updated