diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb
index 9a4ae99c6002e50e243af2806e6baad9ab156c52..45944401c2d30b4eaf14db8f1517fbcb8486d958 100644
--- a/app/models/concerns/cache_markdown_field.rb
+++ b/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
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 7624a1a4e80cf20ab49bb52944d0c30c2a8b0807..137f6e51dad9f3a0c80ab34e88eb80698218309d 100644
--- a/app/models/concerns/issuable.rb
+++ b/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) }
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index b10e8547e8609276bba47f69eed040db160d3b0c..5db077c178de7f5d8da38ba8ed09b4da27a0b7d1 100644
--- a/app/models/concerns/mentionable.rb
+++ b/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
diff --git a/app/models/note.rb b/app/models/note.rb
index cfdac6c432fd0ca19359b31ae0923cd562250b09..77f7726079c2a73c25ad1255f445f8281ecdee96 100644
--- a/app/models/note.rb
+++ b/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)
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index 2e1a2e8e2b2c4f2a616271ab98b3d4db6df35101..1a985ad10678e800bc0e0a1ace89b24e49f2d5bd 100644
--- a/app/models/snippet.rb
+++ b/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
diff --git a/changelogs/unreleased/save-mentions-on-markdown-columns-change.yml b/changelogs/unreleased/save-mentions-on-markdown-columns-change.yml
new file mode 100644
index 0000000000000000000000000000000000000000..7358528219e51d7efd15d902a45bfb921efc0f38
--- /dev/null
+++ b/changelogs/unreleased/save-mentions-on-markdown-columns-change.yml
@@ -0,0 +1,5 @@
+---
+title: Update user mentions when markdown columns are directly saved to DB
+merge_request: 38034
+author:
+type: fixed
diff --git a/lib/gitlab/markdown_cache/active_record/extension.rb b/lib/gitlab/markdown_cache/active_record/extension.rb
index 233d3bf1ac79f6e16d1f9251141e025d669204f9..1de890c84f9558a2491f4f76fc53229d11b71d74 100644
--- a/lib/gitlab/markdown_cache/active_record/extension.rb
+++ b/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).
diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb
index b203c6e606a138b7a9404650fe1e60aef6eaa1b3..6e62d4ef31b9be462359e2408e4428754ddc06b3 100644
--- a/spec/models/concerns/cache_markdown_field_spec.rb
+++ b/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
diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb
index 516c0fd75bcbf8a4cb2291aa2901218cb03188d7..3c095477ea9e40acf0405aee90960f9c1d856d95 100644
--- a/spec/models/concerns/mentionable_spec.rb
+++ b/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
diff --git a/spec/support/shared_examples/models/mentionable_shared_examples.rb b/spec/support/shared_examples/models/mentionable_shared_examples.rb
index 0ee0b7e6d889ea89c9615a7bf2fd0e4dd948dfc0..2392658e5844d734196a0b8860ac2a7a6327a13a 100644
--- a/spec/support/shared_examples/models/mentionable_shared_examples.rb
+++ b/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)