Save user mentions when markdown fields are saved

Move user mentions saving as a callback on models that save related
markdown field cache(_html)

app/models/concerns/cache_markdown_field.rb

@@ -70,10 +70,11 @@ module CacheMarkdownField
 
   def refresh_markdown_cache!
     updates = refresh_markdown_cache
-    if save_markdown(updates)
-      # save_markdown updates DB columns directly, so reload to let store_mentions! be able to parse mentions
-      # otherwise we end up in a loop
-      reset.store_mentions! if is_a?(Mentionable) && mentionable_attrs_changed?(updates.keys)
+    if updates.present? && save_markdown(updates)
+      # save_markdown updates DB columns directly, so compute and save mentions
+      # by calling store_mentions! or we end-up with missing mentions although those
+      # would appear in the notes, descriptions, etc in the UI
+      store_mentions! if mentionable_attributes_changed?(updates)
     end
   end
 
@@ -110,14 +111,15 @@ module CacheMarkdownField
     return unless cached_markdown_fields.markdown_fields.include?(markdown_field)
 
     if attribute_invalidated?(cached_markdown_fields.html_field(markdown_field))
-      # * If html is invalid and there is a change in markdown field, then just refresh html for the
-      # corresponding markdown field.
-      # * Else if the the markdown did not changee it means it is freshly loaded from DB
-      # but its corresponding html field is invalid, so we refresh and update it in DB.
-      # * The change in html field will trigger also a mentions parsing as well as an update if needed.
-      if changed_attributes[markdown_field]
+      # Invalidated due to Markdown content change
+      # We should not persist the updated HTML here since this will depend on whether the
+      # Markdown content change will be persisted. Both will be persisted together when the model is saved.
+      if changed_attributes.key?(markdown_field)
         refresh_markdown_cache
       else
+        # Invalidated due to stale HTML cache
+        # This could happen when the Markdown cache version is bumped or when a model is imported and the HTML is empty.
+        # We persist the updated HTML here so that subsequent calls to this method do not have to regenerate the HTML again.
         refresh_markdown_cache!
       end
     end
@@ -154,13 +156,43 @@ module CacheMarkdownField
     nil
   end
 
-  private
+  def store_mentions!
+    refs = all_references(self.author)
 
-  def mentionable_attrs_changed?(updated_fields)
+    references = {}
+    references[:mentioned_users_ids] = refs.mentioned_users&.pluck(:id).presence
+    references[:mentioned_groups_ids] = refs.mentioned_groups&.pluck(:id).presence
+    references[:mentioned_projects_ids] = refs.mentioned_projects&.pluck(:id).presence
+
+    # One retry is enough as next time `model_user_mention` should return the existing mention record,
+    # that threw the `ActiveRecord::RecordNotUnique` exception in first place.
+    self.class.safe_ensure_unique(retries: 1) do
+      user_mention = model_user_mention
+
+      # this may happen due to notes polymorphism, so noteable_id may point to a record
+      # that no longer exists as we cannot have FK on noteable_id
+      break if user_mention.blank?
+
+      user_mention.mentioned_users_ids = references[:mentioned_users_ids]
+      user_mention.mentioned_groups_ids = references[:mentioned_groups_ids]
+      user_mention.mentioned_projects_ids = references[:mentioned_projects_ids]
+
+      if user_mention.has_mentions?
+        user_mention.save!
+      else
+        user_mention.destroy!
+      end
+    end
+
+    true
+  end
+
+  def mentionable_attributes_changed?(changes = saved_changes)
     return false unless is_a?(Mentionable)
 
     self.class.mentionable_attrs.any? do |attr|
-      updated_fields.include?(cached_markdown_fields.html_field(attr.first))
+      changes.key?(cached_markdown_fields.html_field(attr.first)) &&
+        changes.fetch(cached_markdown_fields.html_field(attr.first)).last.present?
     end
   end
 

app/models/concerns/issuable.rb

@@ -84,7 +84,6 @@ module Issuable
     validate :description_max_length_for_new_records_is_valid, on: :update
 
     before_validation :truncate_description_on_import!
-    after_save :store_mentions!, if: :any_mentionable_attributes_changed?
 
     scope :authored, ->(user) { where(author_id: user) }
     scope :recent, -> { reorder(id: :desc) }

app/models/concerns/mentionable.rb

@@ -80,37 +80,6 @@ module Mentionable
     all_references(current_user).users
   end
 
-  def store_mentions!
-    refs = all_references(self.author)
-
-    references = {}
-    references[:mentioned_users_ids] = refs.mentioned_users&.pluck(:id).presence
-    references[:mentioned_groups_ids] = refs.mentioned_groups&.pluck(:id).presence
-    references[:mentioned_projects_ids] = refs.mentioned_projects&.pluck(:id).presence
-
-    # One retry should be enough as next time `model_user_mention` should return the existing mention record, that
-    # threw the `ActiveRecord::RecordNotUnique` exception in first place.
-    self.class.safe_ensure_unique(retries: 1) do
-      user_mention = model_user_mention
-
-      # this may happen due to notes polymorphism, so noteable_id may point to a record that no longer exists
-      # as we cannot have FK on noteable_id
-      break if user_mention.blank?
-
-      user_mention.mentioned_users_ids = references[:mentioned_users_ids]
-      user_mention.mentioned_groups_ids = references[:mentioned_groups_ids]
-      user_mention.mentioned_projects_ids = references[:mentioned_projects_ids]
-
-      if user_mention.has_mentions?
-        user_mention.save!
-      else
-        user_mention.destroy!
-      end
-    end
-
-    true
-  end
-
   def referenced_users
     User.where(id: user_mentions.select("unnest(mentioned_users_ids)"))
   end
@@ -216,12 +185,6 @@ module Mentionable
     source.select { |key, val| mentionable.include?(key) }
   end
 
-  def any_mentionable_attributes_changed?
-    self.class.mentionable_attrs.any? do |attr|
-      saved_changes.key?(attr.first)
-    end
-  end
-
   # Determine whether or not a cross-reference Note has already been created between this Mentionable and
   # the specified target.
   def cross_reference_exists?(target)
@@ -237,12 +200,12 @@ module Mentionable
   end
 
   # User mention that is parsed from model description rather then its related notes.
-  # Models that have a descriprion attribute like Issue, MergeRequest, Epic, Snippet may have such a user mention.
+  # Models that have a description attribute like Issue, MergeRequest, Epic, Snippet may have such a user mention.
   # Other mentionable models like Commit, DesignManagement::Design, will never have such record as those do not have
   # a description attribute.
   #
   # Using this method followed by a call to *save* may result in *ActiveRecord::RecordNotUnique* exception
-  # in a multithreaded environment. Make sure to use it within a *safe_ensure_unique* block.
+  # in a multi-threaded environment. Make sure to use it within a *safe_ensure_unique* block.
   def model_user_mention
     user_mentions.where(note_id: nil).first_or_initialize
   end

app/models/note.rb

@@ -145,7 +145,6 @@ class Note < ApplicationRecord
   after_save :expire_etag_cache, unless: :importing?
   after_save :touch_noteable, unless: :importing?
   after_destroy :expire_etag_cache
-  after_save :store_mentions!, if: :any_mentionable_attributes_changed?
   after_commit :notify_after_create, on: :create
   after_commit :notify_after_destroy, on: :destroy
 
@@ -548,8 +547,8 @@ class Note < ApplicationRecord
 
   private
 
-  # Using this method followed by a call to `save` may result in ActiveRecord::RecordNotUnique exception
-  # in a multithreaded environment. Make sure to use it within a `safe_ensure_unique` block.
+  # Using this method followed by a call to *save* may result in *ActiveRecord::RecordNotUnique* exception
+  # in a multi-threaded environment. Make sure to use it within a *safe_ensure_unique* block.
   def model_user_mention
     return if user_mentions.is_a?(ActiveRecord::NullRelation)
 

app/models/snippet.rb

@@ -69,7 +69,6 @@ class Snippet < ApplicationRecord
 
   validates :visibility_level, inclusion: { in: Gitlab::VisibilityLevel.values }
 
-  after_save :store_mentions!, if: :any_mentionable_attributes_changed?
   after_create :create_statistics
 
   # Scopes

changelogs/unreleased/save-mentions-on-markdown-columns-change.yml

+---
+title: Update user mentions when markdown columns are directly saved to DB
+merge_request: 38034
+author:
+type: fixed

lib/gitlab/markdown_cache/active_record/extension.rb

@@ -10,6 +10,7 @@ module Gitlab
           # Using before_update here conflicts with elasticsearch-model somehow
           before_create :refresh_markdown_cache, if: :invalidated_markdown_cache?
           before_update :refresh_markdown_cache, if: :invalidated_markdown_cache?
+          after_save :store_mentions!, if: :mentionable_attributes_changed?
         end
 
         # Always exclude _html fields from attributes (including serialization).

spec/models/concerns/cache_markdown_field_spec.rb

@@ -145,7 +145,7 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do
       it 'saves the changes' do
         expect(thing)
           .to receive(:save_markdown)
-                .with("description_html" => updated_html, "title_html" => "", "cached_markdown_version" => cache_version)
+          .with("description_html" => updated_html, "title_html" => "", "cached_markdown_version" => cache_version)
 
         thing.refresh_markdown_cache!
       end
@@ -233,7 +233,7 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do
         end
 
         it 'calls #refresh_markdown_cache!' do
-          expect(thing).to receive(:refresh_markdown_cache!)
+          expect(thing).to receive(:refresh_markdown_cache)
 
           expect(thing.updated_cached_html_for(:description)).to eq(html)
         end
@@ -280,57 +280,89 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do
   end
 
   shared_examples 'a class with mentionable markdown fields' do
-    context 'when klass is a Mentionable' do
+    let(:mentionable) { klass.new(description: markdown, description_html: html, title: markdown, title_html: html, cached_markdown_version: cache_version) }
+
+    context 'when klass is a Mentionable', :aggregate_failures do
       before do
         klass.send(:include, Mentionable)
         klass.send(:attr_mentionable, :description)
       end
 
-      describe '#updated_cached_html_for' do
-        let(:thing) { klass.new(description: markdown, description_html: html, title: markdown, title_html: html, cached_markdown_version: cache_version) }
+      describe '#mentionable_attributes_changed?' do
+        message = Struct.new(:text)
+
+        let(:changes) do
+          msg = message.new('test')
+
+          changes = {}
+          changes[msg] = ['', 'some message']
+          changes[:random_sym_key] = ['', 'some message']
+          changes["description"] = ['', 'some message']
+          changes
+        end
+
+        it 'returns true with key string' do
+          changes["description_html"] = ['', 'some message']
+
+          allow(mentionable).to receive(:saved_changes).and_return(changes)
+
+          expect(mentionable.send(:mentionable_attributes_changed?)).to be true
+        end
+
+        it 'returns false with key symbol' do
+          changes[:description_html] = ['', 'some message']
+          allow(mentionable).to receive(:saved_changes).and_return(changes)
+
+          expect(mentionable.send(:mentionable_attributes_changed?)).to be false
+        end
+
+        it 'returns false when no attr_mentionable keys' do
+          allow(mentionable).to receive(:saved_changes).and_return(changes)
+
+          expect(mentionable.send(:mentionable_attributes_changed?)).to be false
+        end
+      end
 
+      describe '#save' do
         context 'when cache is outdated' do
           before do
             thing.cached_markdown_version += 1
-            allow(thing).to receive(:reset).and_return(thing)
-            allow(thing).to receive(:save_markdown).and_return(true)
           end
 
           context 'when the markdown field also a mentionable attribute' do
+            let(:thing) { klass.new(description: markdown, description_html: html, cached_markdown_version: cache_version) }
+
             it 'calls #store_mentions!' do
+              expect(thing).to receive(:mentionable_attributes_changed?).and_return(true)
               expect(thing).to receive(:store_mentions!)
 
-              expect(thing.updated_cached_html_for(:description)).to eq(html)
+              thing.try(:save)
+
+              expect(thing.description_html).to eq(html)
             end
           end
 
           context 'when the markdown field is not mentionable attribute' do
+            let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) }
+
             it 'does not call #store_mentions!' do
               expect(thing).not_to receive(:store_mentions!)
-              expect(thing).to receive(:refresh_markdown_cache!)
+              expect(thing).to receive(:refresh_markdown_cache)
+
+              thing.try(:save)
 
-              thing.updated_cached_html_for(:title)
+              expect(thing.title_html).to eq(html)
             end
           end
         end
 
         context 'when the markdown field does not exist' do
-          it 'does not call #store_mentions!' do
-            expect(thing).not_to receive(:store_mentions!)
-
-            thing.updated_cached_html_for(:something)
-          end
-        end
-
-        context 'when the markdown cache is up to date' do
-          before do
-            thing.try(:save)
-          end
+          let(:thing) { klass.new(cached_markdown_version: cache_version) }
 
           it 'does not call #store_mentions!' do
             expect(thing).not_to receive(:store_mentions!)
 
-            thing.updated_cached_html_for(:description)
+            thing.try(:save)
           end
         end
       end
@@ -396,6 +428,5 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do
     let(:klass) { other_class }
 
     it_behaves_like 'a class with cached markdown fields'
-    it_behaves_like 'a class with mentionable markdown fields'
   end
 end

spec/models/concerns/mentionable_spec.rb

@@ -29,42 +29,6 @@ RSpec.describe Mentionable do
       expect(mentionable.referenced_mentionables).to be_empty
     end
   end
-
-  describe '#any_mentionable_attributes_changed?' do
-    message = Struct.new(:text)
-
-    let(:mentionable) { Example.new }
-    let(:changes) do
-      msg = message.new('test')
-
-      changes = {}
-      changes[msg] = ['', 'some message']
-      changes[:random_sym_key] = ['', 'some message']
-      changes["random_string_key"] = ['', 'some message']
-      changes
-    end
-
-    it 'returns true with key string' do
-      changes["message"] = ['', 'some message']
-
-      allow(mentionable).to receive(:saved_changes).and_return(changes)
-
-      expect(mentionable.send(:any_mentionable_attributes_changed?)).to be true
-    end
-
-    it 'returns false with key symbol' do
-      changes[:message] = ['', 'some message']
-      allow(mentionable).to receive(:saved_changes).and_return(changes)
-
-      expect(mentionable.send(:any_mentionable_attributes_changed?)).to be false
-    end
-
-    it 'returns false when no attr_mentionable keys' do
-      allow(mentionable).to receive(:saved_changes).and_return(changes)
-
-      expect(mentionable.send(:any_mentionable_attributes_changed?)).to be false
-    end
-  end
 end
 
 RSpec.describe Issue, "Mentionable" do

spec/support/shared_examples/models/mentionable_shared_examples.rb

@@ -92,7 +92,7 @@ RSpec.shared_examples 'a mentionable' do
         end
       end
 
-      expect(subject).to receive(:cached_markdown_fields).at_least(:once).and_call_original
+      expect(subject).to receive(:cached_markdown_fields).at_least(1).and_call_original
 
       subject.all_references(author)
     end
@@ -151,7 +151,7 @@ RSpec.shared_examples 'an editable mentionable' do
       end
 
       it 'persists the refreshed cache so that it does not have to be refreshed every time' do
-        expect(subject).to receive(:refresh_markdown_cache).once.and_call_original
+        expect(subject).to receive(:refresh_markdown_cache).at_least(1).and_call_original
 
         subject.all_references(author)