Merge branch 'trigger-store-mentions-in-after-save-callback' into 'master'

Store mentions in after_save callback

See merge request gitlab-org/gitlab!24586

app/models/concerns/issuable.rb

@@ -91,6 +91,7 @@ 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

@@ -99,18 +99,23 @@ module Mentionable
     # 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!
-      elsif user_mention.persisted?
+      else
         user_mention.destroy!
       end
-
-      true
     end
+
+    true
   end
 
   def referenced_users
@@ -218,6 +223,12 @@ 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)

app/models/issue.rb

@@ -45,7 +45,7 @@ class Issue < ApplicationRecord
   has_many :issue_assignees
   has_many :assignees, class_name: "User", through: :issue_assignees
   has_many :zoom_meetings
-  has_many :user_mentions, class_name: "IssueUserMention"
+  has_many :user_mentions, class_name: "IssueUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
   has_one :sentry_issue
 
   accepts_nested_attributes_for :sentry_issue

app/models/merge_request.rb

@@ -77,7 +77,7 @@ class MergeRequest < ApplicationRecord
 
   has_many :merge_request_assignees
   has_many :assignees, class_name: "User", through: :merge_request_assignees
-  has_many :user_mentions, class_name: "MergeRequestUserMention"
+  has_many :user_mentions, class_name: "MergeRequestUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :deployment_merge_requests
 

app/models/note.rb

@@ -157,6 +157,7 @@ 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?
 
   class << self
     def model_name
@@ -498,6 +499,8 @@ class Note < ApplicationRecord
   end
 
   def user_mentions
+    return Note.none unless noteable.present?
+
     noteable.user_mentions.where(note: self)
   end
 
@@ -506,6 +509,8 @@ class Note < ApplicationRecord
   # 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.
   def model_user_mention
+    return if user_mentions.is_a?(ActiveRecord::NullRelation)
+
     user_mentions.first_or_initialize
   end
 

app/models/snippet.rb

@@ -41,7 +41,7 @@ class Snippet < ApplicationRecord
   belongs_to :project
 
   has_many :notes, as: :noteable, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :user_mentions, class_name: "SnippetUserMention"
+  has_many :user_mentions, class_name: "SnippetUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
   has_one :snippet_repository, inverse_of: :snippet
 
   delegate :name, :email, to: :author, prefix: true, allow_nil: true
@@ -66,6 +66,8 @@ class Snippet < ApplicationRecord
 
   validates :visibility_level, inclusion: { in: Gitlab::VisibilityLevel.values }
 
+  after_save :store_mentions!, if: :any_mentionable_attributes_changed?
+
   # Scopes
   scope :are_internal, -> { where(visibility_level: Snippet::INTERNAL) }
   scope :are_private, -> { where(visibility_level: Snippet::PRIVATE) }

app/services/issuable_base_service.rb

@@ -168,7 +168,7 @@ class IssuableBaseService < BaseService
     before_create(issuable)
 
     issuable_saved = issuable.with_transaction_returning_status do
-      issuable.save && issuable.store_mentions!
+      issuable.save
     end
 
     if issuable_saved
@@ -233,7 +233,7 @@ class IssuableBaseService < BaseService
       ensure_milestone_available(issuable)
 
       issuable_saved = issuable.with_transaction_returning_status do
-        issuable.save(touch: should_touch) && issuable.store_mentions!
+        issuable.save(touch: should_touch)
       end
 
       if issuable_saved

app/services/notes/create_service.rb

@@ -2,7 +2,6 @@
 
 module Notes
   class CreateService < ::Notes::BaseService
-    # rubocop:disable Metrics/CyclomaticComplexity
     def execute
       note = Notes::BuildService.new(project, current_user, params.except(:merge_request_diff_head_sha)).execute
 
@@ -34,7 +33,7 @@ module Notes
       end
 
       note_saved = note.with_transaction_returning_status do
-        !only_commands && note.save && note.store_mentions!
+        !only_commands && note.save
       end
 
       if note_saved
@@ -67,7 +66,6 @@ module Notes
 
       note
     end
-    # rubocop:enable Metrics/CyclomaticComplexity
 
     private
 

app/services/notes/update_service.rb

@@ -10,7 +10,7 @@ module Notes
       note.assign_attributes(params.merge(updated_by: current_user))
 
       note.with_transaction_returning_status do
-        note.save && note.store_mentions!
+        note.save
       end
 
       only_commands = false

app/services/snippets/create_service.rb

@@ -24,7 +24,7 @@ module Snippets
       spam_check(snippet, current_user)
 
       snippet_saved = snippet.with_transaction_returning_status do
-        snippet.save && snippet.store_mentions!
+        snippet.save
       end
 
       if snippet_saved

app/services/snippets/update_service.rb

@@ -21,7 +21,7 @@ module Snippets
       spam_check(snippet, current_user)
 
       snippet_saved = snippet.with_transaction_returning_status do
-        snippet.save && snippet.store_mentions!
+        snippet.save
       end
 
       if snippet_saved

ee/app/models/design_management/design.rb

@@ -17,7 +17,7 @@ module DesignManagement
     # This is a polymorphic association, so we can't count on FK's to delete the
     # data
     has_many :notes, as: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
-    has_many :user_mentions, class_name: "DesignUserMention"
+    has_many :user_mentions, class_name: "DesignUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
     validates :project, :filename, presence: true
     validates :issue, presence: true, unless: :importing?

ee/app/models/ee/epic.rb

@@ -52,7 +52,7 @@ module EE
 
       has_many :epic_issues
       has_many :issues, through: :epic_issues
-      has_many :user_mentions, class_name: "EpicUserMention"
+      has_many :user_mentions, class_name: "EpicUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
       validates :group, presence: true
       validate :validate_parent, on: :create

ee/spec/requests/api/epics_spec.rb

@@ -677,6 +677,8 @@ describe API::Epics do
         end
 
         it 'updates the epic with labels param as array' do
+          stub_const("Gitlab::QueryLimiting::Transaction::THRESHOLD", 110)
+
           params[:labels] = ['label1', 'label2', 'foo, bar', '&,?']
 
           put api(url, user), params: params

ee/spec/services/epic_issues/create_service_spec.rb

@@ -123,6 +123,10 @@ describe EpicIssues::CreateService do
               # Extractor makes a permission check for each issue which messes up the query count check
               extractor = double
               allow(Gitlab::ReferenceExtractor).to receive(:new).and_return(extractor)
+              allow(extractor).to receive(:reset_memoized_values)
+              allow(extractor).to receive(:mentioned_users)
+              allow(extractor).to receive(:mentioned_groups)
+              allow(extractor).to receive(:mentioned_projects)
               allow(extractor).to receive(:analyze)
               allow(extractor).to receive(:issues).and_return([issue])
 

ee/spec/services/epics/create_service_spec.rb

@@ -40,4 +40,47 @@ describe Epics::CreateService do
       expect(epic.start_date_is_fixed).to be_truthy
     end
   end
+
+  context 'after_save callback to store_mentions' do
+    let(:labels) { create_pair(:group_label, group: group) }
+
+    context 'when mentionable attributes change' do
+      context 'when content has no mentions' do
+        let(:params) { { title: 'Title', description: "Description with no mentions" } }
+
+        it 'calls store_mentions! and saves no mentions' do
+          expect_next_instance_of(Epic) do |instance|
+            expect(instance).to receive(:store_mentions!).and_call_original
+          end
+
+          expect { subject }.not_to change { EpicUserMention.count }
+        end
+      end
+
+      context 'when content has mentions' do
+        let(:params) { { title: 'Title', description: "Description with #{user.to_reference}" } }
+
+        it 'calls store_mentions! and saves mentions' do
+          expect_next_instance_of(Epic) do |instance|
+            expect(instance).to receive(:store_mentions!).and_call_original
+          end
+
+          expect { subject }.to change { EpicUserMention.count }.by(1)
+        end
+      end
+
+      context 'when mentionable.save fails' do
+        let(:params) { { title: '', label_ids: labels.map(&:id) } }
+
+        it 'does not call store_mentions and saves no mentions' do
+          expect_next_instance_of(Epic) do |instance|
+            expect(instance).not_to receive(:store_mentions!).and_call_original
+          end
+
+          expect { subject }.not_to change { EpicUserMention.count }
+          expect(subject.valid?).to be false
+        end
+      end
+    end
+  end
 end

ee/spec/services/epics/issue_promote_service_spec.rb

@@ -55,6 +55,9 @@ describe Epics::IssuePromoteService do
         end
 
         context 'when issue is promoted' do
+          let!(:issue_mentionable_note) { create(:note, noteable: issue, author: user, project: project, note: "note with mention #{user.to_reference}") }
+          let!(:issue_note) { create(:note, noteable: issue, author: user, project: project, note: "note without mention") }
+
           before do
             allow(Gitlab::Tracking).to receive(:event).with('epics', 'promote', an_instance_of(Hash))
 
@@ -88,6 +91,17 @@ describe Epics::IssuePromoteService do
             expect(issue).to be_promoted
             expect(issue.promoted_to_epic).to eq(epic)
           end
+
+          context 'when issue description has mentions and has notes with mentions' do
+            let(:issue) { create(:issue, project: project, description: "description with mention to #{user.to_reference}") }
+
+            it 'only saves user mentions with actual mentions' do
+              expect(epic.user_mentions.where(note_id: nil).first.mentioned_users_ids).to match_array([user.id])
+              expect(epic.user_mentions.where.not(note_id: nil).first.mentioned_users_ids).to match_array([user.id])
+              expect(epic.user_mentions.where.not(note_id: nil).count).to eq 1
+              expect(epic.user_mentions.count).to eq 2
+            end
+          end
         end
 
         context 'when promoted issue has notes' do

ee/spec/services/epics/update_service_spec.rb

@@ -82,6 +82,49 @@ describe Epics::UpdateService do
       end
     end
 
+    context 'after_save callback to store_mentions' do
+      let(:user2) { create(:user) }
+      let(:epic) { create(:epic, group: group, description: "simple description") }
+      let(:labels) { create_pair(:group_label, group: group) }
+
+      context 'when mentionable attributes change' do
+        let(:opts) { { description: "Description with #{user.to_reference}" } }
+
+        it 'saves mentions' do
+          expect(epic).to receive(:store_mentions!).and_call_original
+
+          expect { update_epic(opts) }.to change { EpicUserMention.count }.by(1)
+
+          expect(epic.referenced_users).to match_array([user])
+        end
+      end
+
+      context 'when mentionable attributes do not change' do
+        let(:opts) { { label_ids: labels.map(&:id) } }
+
+        it 'does not call store_mentions!' do
+          expect(epic).not_to receive(:store_mentions!).and_call_original
+
+          expect { update_epic(opts) }.not_to change { EpicUserMention.count }
+
+          expect(epic.referenced_users).to be_empty
+        end
+      end
+
+      context 'when save fails' do
+        let(:opts) { { title: '', label_ids: labels.map(&:id) } }
+
+        it 'does not call store_mentions!' do
+          expect(epic).not_to receive(:store_mentions!).and_call_original
+
+          expect { update_epic(opts) }.not_to change { EpicUserMention.count }
+
+          expect(epic.referenced_users).to be_empty
+          expect(epic.valid?).to be false
+        end
+      end
+    end
+
     context 'todos' do
       before do
         group.update(visibility: Gitlab::VisibilityLevel::PUBLIC)

spec/features/cycle_analytics_spec.rb

@@ -40,9 +40,6 @@ describe 'Value Stream Analytics', :js do
 
     context "when there's value stream analytics data" do
       before do
-        allow_next_instance_of(Gitlab::ReferenceExtractor) do |instance|
-          allow(instance).to receive(:issues).and_return([issue])
-        end
         project.add_maintainer(user)
 
         @build = create_cycle(user, project, issue, mr, milestone, pipeline)
@@ -101,9 +98,6 @@ describe 'Value Stream Analytics', :js do
       project.add_developer(user)
       project.add_guest(guest)
 
-      allow_next_instance_of(Gitlab::ReferenceExtractor) do |instance|
-        allow(instance).to receive(:issues).and_return([issue])
-      end
       create_cycle(user, project, issue, mr, milestone, pipeline)
       deploy_master(user, project)
 

spec/lib/gitlab/import_export/fast_hash_serializer_spec.rb

@@ -216,8 +216,6 @@ describe Gitlab::ImportExport::FastHashSerializer do
   end
 
   def setup_project
-    issue = create(:issue, assignees: [user])
-    snippet = create(:project_snippet)
     release = create(:release)
     group = create(:group)
 
@@ -228,12 +226,14 @@ describe Gitlab::ImportExport::FastHashSerializer do
                      :wiki_enabled,
                      :builds_private,
                      description: 'description',
-                     issues: [issue],
-                     snippets: [snippet],
                      releases: [release],
                      group: group,
                      approvals_before_merge: 1
                     )
+    allow(project).to receive(:commit).and_return(Commit.new(RepoHelpers.sample_commit, project))
+
+    issue = create(:issue, assignees: [user], project: project)
+    snippet = create(:project_snippet, project: project)
     project_label = create(:label, project: project)
     group_label = create(:group_label, group: group)
     create(:label_link, label: project_label, target: issue)

spec/lib/gitlab/import_export/project_tree_saver_spec.rb

@@ -334,8 +334,6 @@ describe Gitlab::ImportExport::ProjectTreeSaver do
   end
 
   def setup_project
-    issue = create(:issue, assignees: [user])
-    snippet = create(:project_snippet)
     release = create(:release)
     group = create(:group)
 
@@ -346,12 +344,14 @@ describe Gitlab::ImportExport::ProjectTreeSaver do
                      :wiki_enabled,
                      :builds_private,
                      description: 'description',
-                     issues: [issue],
-                     snippets: [snippet],
                      releases: [release],
                      group: group,
                      approvals_before_merge: 1
                     )
+    allow(project).to receive(:commit).and_return(Commit.new(RepoHelpers.sample_commit, project))
+
+    issue = create(:issue, assignees: [user], project: project)
+    snippet = create(:project_snippet, project: project)
     project_label = create(:label, project: project)
     group_label = create(:group_label, group: group)
     create(:label_link, label: project_label, target: issue)

spec/models/concerns/mentionable_spec.rb

@@ -26,6 +26,42 @@ 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
 
 describe Issue, "Mentionable" do

spec/models/cycle_analytics/group_level_spec.rb

@@ -22,10 +22,6 @@ describe CycleAnalytics::GroupLevel do
 
   describe '#stats' do
     before do
-      allow_next_instance_of(Gitlab::ReferenceExtractor) do |instance|
-        allow(instance).to receive(:issues).and_return([issue])
-      end
-
       create_cycle(user, project, issue, mr, milestone, pipeline)
       deploy_master(user, project)
     end

spec/models/event_spec.rb

@@ -173,6 +173,7 @@ describe Event do
     end
 
     context 'commit note event' do
+      let(:project) { create(:project, :public, :repository) }
       let(:target) { note_on_commit }
 
       it do
@@ -185,7 +186,7 @@ describe Event do
       end
 
       context 'private project' do
-        let(:project) { create(:project, :private) }
+        let(:project) { create(:project, :private, :repository) }
 
         it do
           aggregate_failures do

spec/models/project_spec.rb

@@ -1299,8 +1299,8 @@ describe Project do
 
   describe '.trending' do
     let(:group)    { create(:group, :public) }
-    let(:project1) { create(:project, :public, group: group) }
-    let(:project2) { create(:project, :public, group: group) }
+    let(:project1) { create(:project, :public, :repository, group: group) }
+    let(:project2) { create(:project, :public, :repository, group: group) }
 
     before do
       create_list(:note_on_commit, 2, project: project1)

spec/models/trending_project_spec.rb

@@ -4,11 +4,11 @@ require 'spec_helper'
 
 describe TrendingProject do
   let(:user) { create(:user) }
-  let(:public_project1) { create(:project, :public) }
-  let(:public_project2) { create(:project, :public) }
-  let(:public_project3) { create(:project, :public) }
-  let(:private_project) { create(:project, :private) }
-  let(:internal_project) { create(:project, :internal) }
+  let(:public_project1) { create(:project, :public, :repository) }
+  let(:public_project2) { create(:project, :public, :repository) }
+  let(:public_project3) { create(:project, :public, :repository) }
+  let(:private_project) { create(:project, :private, :repository) }
+  let(:internal_project) { create(:project, :internal, :repository) }
 
   before do
     create_list(:note_on_commit, 3, project: public_project1)

spec/services/issues/create_service_spec.rb

@@ -171,6 +171,31 @@ describe Issues::CreateService do
 
         described_class.new(project, user, opts).execute
       end
+
+      context 'after_save callback to store_mentions' do
+        context 'when mentionable attributes change' do
+          let(:opts) { { title: 'Title', description: "Description with #{user.to_reference}" } }
+
+          it 'saves mentions' do
+            expect_next_instance_of(Issue) do |instance|
+              expect(instance).to receive(:store_mentions!).and_call_original
+            end
+            expect(issue.user_mentions.count).to eq 1
+          end
+        end
+
+        context 'when save fails' do
+          let(:opts) { { title: '', label_ids: labels.map(&:id), milestone_id: milestone.id } }
+
+          it 'does not call store_mentions' do
+            expect_next_instance_of(Issue) do |instance|
+              expect(instance).not_to receive(:store_mentions!).and_call_original
+            end
+            expect(issue.valid?).to be false
+            expect(issue.user_mentions.count).to eq 0
+          end
+        end
+      end
     end
 
     context 'issue create service' do

spec/services/issues/move_service_spec.rb

@@ -6,7 +6,7 @@ describe Issues::MoveService do
   let(:user) { create(:user) }
   let(:author) { create(:user) }
   let(:title) { 'Some issue' }
-  let(:description) { 'Some issue description' }
+  let(:description) { "Some issue description with mention to #{user.to_reference}" }
   let(:group) { create(:group, :private) }
   let(:sub_group_1) { create(:group, :private, parent: group) }
   let(:sub_group_2) { create(:group, :private, parent: group) }
@@ -36,6 +36,9 @@ describe Issues::MoveService do
     end
 
     context 'issue movable' do
+      let!(:note_with_mention) { create(:note, noteable: old_issue, author: author, project: old_project, note: "note with mention #{user.to_reference}") }
+      let!(:note_with_no_mention) { create(:note, noteable: old_issue, author: author, project: old_project, note: "note without mention") }
+
       include_context 'user can move issue'
 
       context 'generic issue' do
@@ -94,6 +97,15 @@ describe Issues::MoveService do
         it 'moves the award emoji' do
           expect(old_issue.award_emoji.first.name).to eq new_issue.reload.award_emoji.first.name
         end
+
+        context 'when issue has notes with mentions' do
+          it 'saves user mentions with actual mentions for new issue' do
+            expect(new_issue.user_mentions.where(note_id: nil).first.mentioned_users_ids).to match_array([user.id])
+            expect(new_issue.user_mentions.where.not(note_id: nil).first.mentioned_users_ids).to match_array([user.id])
+            expect(new_issue.user_mentions.where.not(note_id: nil).count).to eq 1
+            expect(new_issue.user_mentions.count).to eq 2
+          end
+        end
       end
 
       context 'issue with assignee' do

spec/services/issues/update_service_spec.rb

@@ -211,6 +211,49 @@ describe Issues::UpdateService, :mailer do
           expect(note.note).to eq 'locked this issue'
         end
       end
+
+      context 'after_save callback to store_mentions' do
+        let(:issue) { create(:issue, title: 'Old title', description: "simple description", project: project, author: create(:user)) }
+        let(:labels) { create_pair(:label, project: project) }
+        let(:milestone) { create(:milestone, project: project) }
+
+        context 'when mentionable attributes change' do
+          let(:opts) { { description: "Description with #{user.to_reference}" } }
+
+          it 'saves mentions' do
+            expect(issue).to receive(:store_mentions!).and_call_original
+
+            expect { update_issue(opts) }.to change { IssueUserMention.count }.by(1)
+
+            expect(issue.referenced_users).to match_array([user])
+          end
+        end
+
+        context 'when mentionable attributes do not change' do
+          let(:opts) { { label_ids: labels.map(&:id), milestone_id: milestone.id } }
+
+          it 'does not call store_mentions' do
+            expect(issue).not_to receive(:store_mentions!).and_call_original
+
+            expect { update_issue(opts) }.not_to change { IssueUserMention.count }
+
+            expect(issue.referenced_users).to be_empty
+          end
+        end
+
+        context 'when save fails' do
+          let(:opts) { { title: '', label_ids: labels.map(&:id), milestone_id: milestone.id } }
+
+          it 'does not call store_mentions' do
+            expect(issue).not_to receive(:store_mentions!).and_call_original
+
+            expect { update_issue(opts) }.not_to change { IssueUserMention.count }
+
+            expect(issue.referenced_users).to be_empty
+            expect(issue.valid?).to be false
+          end
+        end
+      end
     end
 
     context 'when description changed' do

spec/services/merge_requests/create_service_spec.rb

@@ -291,6 +291,46 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
         expect { service.execute }.to change { counter.read(:create) }.by(1)
       end
+
+      context 'after_save callback to store_mentions' do
+        let(:labels) { create_pair(:label, project: project) }
+        let(:milestone) { create(:milestone, project: project) }
+        let(:req_opts) { { source_branch: 'feature', target_branch: 'master' } }
+
+        context 'when mentionable attributes change' do
+          let(:opts) { { title: 'Title', description: "Description with #{user.to_reference}" }.merge(req_opts) }
+
+          it 'saves mentions' do
+            expect_next_instance_of(MergeRequest) do |instance|
+              expect(instance).to receive(:store_mentions!).and_call_original
+            end
+            expect(merge_request.user_mentions.count).to eq 1
+          end
+        end
+
+        context 'when mentionable attributes do not change' do
+          let(:opts) { { label_ids: labels.map(&:id), milestone_id: milestone.id }.merge(req_opts) }
+
+          it 'does not call store_mentions' do
+            expect_next_instance_of(MergeRequest) do |instance|
+              expect(instance).not_to receive(:store_mentions!).and_call_original
+            end
+            expect(merge_request.valid?).to be false
+            expect(merge_request.user_mentions.count).to eq 0
+          end
+        end
+
+        context 'when save fails' do
+          let(:opts) { { label_ids: labels.map(&:id), milestone_id: milestone.id } }
+
+          it 'does not call store_mentions' do
+            expect_next_instance_of(MergeRequest) do |instance|
+              expect(instance).not_to receive(:store_mentions!).and_call_original
+            end
+            expect(merge_request.valid?).to be false
+          end
+        end
+      end
     end
 
     it_behaves_like 'new issuable record that supports quick actions' do

spec/services/merge_requests/update_service_spec.rb

@@ -162,6 +162,52 @@ describe MergeRequests::UpdateService, :mailer do
       end
     end
 
+    context 'after_save callback to store_mentions' do
+      let(:merge_request) { create(:merge_request, title: 'Old title', description: "simple description", source_branch: 'test', source_project: project, author: user) }
+      let(:labels) { create_pair(:label, project: project) }
+      let(:milestone) { create(:milestone, project: project) }
+      let(:req_opts) { { source_branch: 'feature', target_branch: 'master' } }
+
+      subject { MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) }
+
+      context 'when mentionable attributes change' do
+        let(:opts) { { description: "Description with #{user.to_reference}" }.merge(req_opts) }
+
+        it 'saves mentions' do
+          expect(merge_request).to receive(:store_mentions!).and_call_original
+
+          expect { subject }.to change { MergeRequestUserMention.count }.by(1)
+
+          expect(merge_request.referenced_users).to match_array([user])
+        end
+      end
+
+      context 'when mentionable attributes do not change' do
+        let(:opts) { { label_ids: [label.id, label2.id], milestone_id: milestone.id }.merge(req_opts) }
+
+        it 'does not call store_mentions' do
+          expect(merge_request).not_to receive(:store_mentions!).and_call_original
+
+          expect { subject }.not_to change { MergeRequestUserMention.count }
+
+          expect(merge_request.referenced_users).to be_empty
+        end
+      end
+
+      context 'when save fails' do
+        let(:opts) { { title: '', label_ids: labels.map(&:id), milestone_id: milestone.id } }
+
+        it 'does not call store_mentions' do
+          expect(merge_request).not_to receive(:store_mentions!).and_call_original
+
+          expect { subject }.not_to change { MergeRequestUserMention.count }
+
+          expect(merge_request.referenced_users).to be_empty
+          expect(merge_request.valid?).to be false
+        end
+      end
+    end
+
     context 'merge' do
       let(:opts) do
         {

spec/support/shared_examples/models/mentionable_shared_examples.rb

@@ -86,7 +86,7 @@ RSpec.shared_examples 'a mentionable' do
     end
 
     it 'sends in cached markdown fields when appropriate' do
-      if subject.is_a?(CacheMarkdownField)
+      if subject.is_a?(CacheMarkdownField) && subject.extractors[author].blank?
         expect_next_instance_of(Gitlab::ReferenceExtractor) do |ext|
           attrs = subject.class.mentionable_attrs.collect(&:first) & subject.cached_markdown_fields.markdown_fields
           attrs.each do |field|
@@ -136,7 +136,7 @@ RSpec.shared_examples 'an editable mentionable' do
 
       set_mentionable_text.call('This is a text')
 
-      if subject.is_a?(CacheMarkdownField)
+      if subject.is_a?(CacheMarkdownField) && subject.extractors[author].blank?
         expect_next_instance_of(Gitlab::ReferenceExtractor) do |ext|
           subject.cached_markdown_fields.markdown_fields.each do |field|
             expect(ext).to receive(:analyze).with(subject.send(field), hash_including(rendered: anything))