Fix cache invalidation for Markdown fields

This removes the extra optimization where we skip rendering when we
see that both the markdown field and the HTML field are both updated.

This is unreliable because even if both fields are changed, we cannot
be sure that the HTML is correct for the current markdown value.

app/models/concerns/cache_markdown_field.rb

@@ -161,7 +161,6 @@ module CacheMarkdownField
       define_method(invalidation_method) do
         changed_fields = changed_attributes.keys
         invalidations  = changed_fields & [markdown_field.to_s, *INVALIDATED_BY]
-        invalidations.delete(markdown_field.to_s) if changed_fields.include?("#{markdown_field}_html")
         !invalidations.empty? || !cached_html_up_to_date?(markdown_field)
       end
     end

changelogs/unreleased/208255-editing-markdown-fields-don-t-work.yml

+---
+title: Fix updating of Markdown fields when Markdown cache version is incremented
+merge_request: 32219
+author:
+type: fixed

spec/models/concerns/cache_markdown_field_spec.rb

@@ -223,6 +223,10 @@ describe CacheMarkdownField, :clean_gitlab_redis_cache do
       end
 
       context 'when the markdown cache is up to date' do
+        before do
+          thing.try(:save)
+        end
+
         it 'does not call #refresh_markdown_cache' do
           expect(thing).not_to receive(:refresh_markdown_cache)
 
@@ -256,6 +260,54 @@ describe CacheMarkdownField, :clean_gitlab_redis_cache do
     let(:klass) { ar_class }
 
     it_behaves_like 'a class with cached markdown fields'
+
+    describe '#attribute_invalidated?' do
+      let(:thing) { klass.create(description: markdown, description_html: html, cached_markdown_version: cache_version) }
+
+      it 'returns true when cached_markdown_version is different' do
+        thing.cached_markdown_version += 1
+
+        expect(thing.attribute_invalidated?(:description_html)).to eq(true)
+      end
+
+      it 'returns true when markdown is changed' do
+        thing.description = updated_markdown
+
+        expect(thing.attribute_invalidated?(:description_html)).to eq(true)
+      end
+
+      it 'returns true when both markdown and HTML are changed' do
+        thing.description = updated_markdown
+        thing.description_html = updated_html
+
+        expect(thing.attribute_invalidated?(:description_html)).to eq(true)
+      end
+
+      it 'returns false when there are no changes' do
+        expect(thing.attribute_invalidated?(:description_html)).to eq(false)
+      end
+    end
+
+    context 'when cache version is updated' do
+      let(:old_version) { cache_version - 1 }
+      let(:old_html) { '<p data-sourcepos="1:1-1:5" dir="auto" class="some-old-class"><code>Foo</code></p>' }
+
+      let(:thing) do
+        # This forces the record to have outdated HTML. We can't use `create` because the `before_create` hook
+        # would re-render the HTML to the latest version
+        klass.create.tap do |thing|
+          thing.update_columns(description: markdown, description_html: old_html, cached_markdown_version: old_version)
+        end
+      end
+
+      it 'correctly updates cached HTML even if refresh_markdown_cache is called before updating the attribute' do
+        thing.refresh_markdown_cache
+
+        thing.update(description: updated_markdown)
+
+        expect(thing.description_html).to eq(updated_html)
+      end
+    end
   end
 
   context 'for other classes' do