Merge branch 'rs-issue-1773' into 'master'

Fix mentions not being created upon issue/merge request update

New cross-references weren't being added when they were made in an issue or merge request update.

This happened because the relevant `UpdateService`s were making the `notice_added_references` call
after the model had already been updated and saved, so the `changes` attribute was empty and no
cross-references were made at all.

This fixes the bug and adds a bit of testing and a bit of refactoring.

Closes #1773

See merge request !974

app/models/concerns/mentionable.rb

@@ -79,22 +79,36 @@ module Mentionable
     end
   end
 
-  # If the mentionable_text field is about to change, locate any *added* references and create cross references for
-  # them. Invoke from an observer's #before_save implementation.
-  def notice_added_references(p = project, a = author)
-    ch = changed_attributes
-    original, mentionable_changed = "", false
-    self.class.mentionable_attrs.each do |attr|
-      if ch[attr]
-        original << ch[attr]
-        mentionable_changed = true
-      end
-    end
+  # When a mentionable field is changed, creates cross-reference notes that
+  # don't already exist
+  def create_new_cross_references!(p = project, a = author)
+    changes = detect_mentionable_changes
+
+    return if changes.empty?
 
-    # Only proceed if the saved changes actually include a chance to an attr_mentionable field.
-    return unless mentionable_changed
+    original_text = changes.collect { |_, vals| vals.first }.join(' ')
 
-    preexisting = references(p, self.author, original)
+    preexisting = references(p, self.author, original_text)
     create_cross_references!(p, a, preexisting)
   end
+
+  private
+
+  # Returns a Hash of changed mentionable fields
+  #
+  # Preference is given to the `changes` Hash, but falls back to
+  # `previous_changes` if it's empty (i.e., the changes have already been
+  # persisted).
+  #
+  # See ActiveModel::Dirty.
+  #
+  # Returns a Hash.
+  def detect_mentionable_changes
+    source = (changes.present? ? changes : previous_changes).dup
+
+    mentionable = self.class.mentionable_attrs
+
+    # Only include changed fields that are mentionable
+    source.select { |key, val| mentionable.include?(key) }
+  end
 end

app/models/note.rb

@@ -356,7 +356,7 @@ class Note < ActiveRecord::Base
   end
 
   def set_references
-    notice_added_references(project, author)
+    create_new_cross_references!(project, author)
   end
 
   def editable?

app/services/issues/update_service.rb

@@ -35,7 +35,7 @@ module Issues
           create_title_change_note(issue, issue.previous_changes['title'].first)
         end
 
-        issue.notice_added_references(issue.project, current_user)
+        issue.create_new_cross_references!(issue.project, current_user)
         execute_hooks(issue, 'update')
       end
 

app/services/merge_requests/update_service.rb

@@ -59,7 +59,7 @@ module MergeRequests
           merge_request.mark_as_unchecked
         end
 
-        merge_request.notice_added_references(merge_request.project, current_user)
+        merge_request.create_new_cross_references!(merge_request.project, current_user)
         execute_hooks(merge_request, 'update')
       end
 

spec/models/concerns/mentionable_spec.rb

@@ -28,4 +28,53 @@ describe Issue, "Mentionable" do
       issue.create_cross_references!(project, author, [commit2])
     end
   end
+
+  describe '#create_new_cross_references!' do
+    let(:project) { create(:project) }
+    let(:issues)  { create_list(:issue, 2, project: project) }
+
+    context 'before changes are persisted' do
+      it 'ignores pre-existing references' do
+        issue = create_issue(description: issues[0].to_reference)
+
+        expect(SystemNoteService).not_to receive(:cross_reference)
+
+        issue.description = 'New description'
+        issue.create_new_cross_references!
+      end
+
+      it 'notifies new references' do
+        issue = create_issue(description: issues[0].to_reference)
+
+        expect(SystemNoteService).to receive(:cross_reference).with(issues[1], any_args)
+
+        issue.description = issues[1].to_reference
+        issue.create_new_cross_references!
+      end
+    end
+
+    context 'after changes are persisted' do
+      it 'ignores pre-existing references' do
+        issue = create_issue(description: issues[0].to_reference)
+
+        expect(SystemNoteService).not_to receive(:cross_reference)
+
+        issue.update_attributes(description: 'New description')
+        issue.create_new_cross_references!
+      end
+
+      it 'notifies new references' do
+        issue = create_issue(description: issues[0].to_reference)
+
+        expect(SystemNoteService).to receive(:cross_reference).with(issues[1], any_args)
+
+        issue.update_attributes(description: issues[1].to_reference)
+        issue.create_new_cross_references!
+      end
+    end
+
+    def create_issue(description:)
+      create(:issue, project: project, description: description)
+    end
+  end
 end

spec/support/mentionable_shared_examples.rb

@@ -143,6 +143,6 @@ shared_examples 'an editable mentionable' do
     end
 
     set_mentionable_text.call(new_text)
-    subject.notice_added_references(project, author)
+    subject.create_new_cross_references!(project, author)
   end
 end