Merge branch 'save-mentions-on-markdown-columns-change' into 'master'

Save mentions when markdown columns are directly saved to DB

See merge request gitlab-org/gitlab!38034

app/models/concerns/cache_markdown_field.rb

@@ -70,8 +70,12 @@ module CacheMarkdownField
 
   def refresh_markdown_cache!
     updates = refresh_markdown_cache
-
-    save_markdown(updates)
+    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
 
   def cached_html_up_to_date?(markdown_field)
@@ -106,7 +110,19 @@ module CacheMarkdownField
   def updated_cached_html_for(markdown_field)
     return unless cached_markdown_fields.markdown_fields.include?(markdown_field)
 
-    refresh_markdown_cache! if attribute_invalidated?(cached_markdown_fields.html_field(markdown_field))
+    if attribute_invalidated?(cached_markdown_fields.html_field(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
 
     cached_html_for(markdown_field)
   end
@@ -140,6 +156,46 @@ module CacheMarkdownField
     nil
   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 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|
+      changes.key?(cached_markdown_fields.html_field(attr.first)) &&
+        changes.fetch(cached_markdown_fields.html_field(attr.first)).last.present?
+    end
+  end
+
   included do
     cattr_reader :cached_markdown_fields do
       Gitlab::MarkdownCache::FieldData.new

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

@@ -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
@@ -279,10 +279,101 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do
     end
   end
 
+  shared_examples 'a class with mentionable markdown fields' 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 '#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
+          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!)
+
+              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)
+
+              thing.try(:save)
+
+              expect(thing.title_html).to eq(html)
+            end
+          end
+        end
+
+        context 'when the markdown field does not exist' do
+          let(:thing) { klass.new(cached_markdown_version: cache_version) }
+
+          it 'does not call #store_mentions!' do
+            expect(thing).not_to receive(:store_mentions!)
+
+            thing.try(:save)
+          end
+        end
+      end
+    end
+  end
+
   context 'for Active record classes' do
     let(:klass) { ar_class }
 
     it_behaves_like 'a class with cached markdown fields'
+    it_behaves_like 'a class with mentionable markdown fields'
 
     describe '#attribute_invalidated?' do
       let(:thing) { klass.create!(description: markdown, description_html: html, cached_markdown_version: cache_version) }

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)