From e4c698fd5ce77e46e3851384c14271eb74c3c9ee Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 13 Jul 2015 18:28:10 -0400 Subject: Refactor Mentionable#notice_added_references It now accounts for models that have changed but have already been persisted, such as when called from an UpdateService. Closes #1773 --- app/models/concerns/mentionable.rb | 36 ++++++++++++++++------- spec/models/concerns/mentionable_spec.rb | 49 ++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 11 deletions(-) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 56849f28ff0..8ff670dd2bf 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -82,19 +82,33 @@ module Mentionable # 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 + 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 diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb index f7f66987b5f..82e8a83bb3d 100644 --- a/spec/models/concerns/mentionable_spec.rb +++ b/spec/models/concerns/mentionable_spec.rb @@ -28,4 +28,53 @@ describe Issue, "Mentionable" do issue.create_cross_references!(project, author, [commit2]) end end + + describe '#notice_added_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.notice_added_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.notice_added_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.notice_added_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.notice_added_references + end + end + + def create_issue(description:) + create(:issue, project: project, description: description) + end + end end -- cgit v1.2.1 From f3d4767d0c78daf315e6b653bed3a3a3ee308072 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 13 Jul 2015 18:33:50 -0400 Subject: Rename `notice_added_references` to `create_new_cross_references!` --- app/models/concerns/mentionable.rb | 6 +++--- app/models/note.rb | 2 +- app/services/issues/update_service.rb | 2 +- app/services/merge_requests/update_service.rb | 2 +- spec/models/concerns/mentionable_spec.rb | 10 +++++----- spec/support/mentionable_shared_examples.rb | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 8ff670dd2bf..5b0ae411642 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -79,9 +79,9 @@ 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) + # 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? diff --git a/app/models/note.rb b/app/models/note.rb index 68b9d433ae0..62567f471dc 100644 --- a/app/models/note.rb +++ b/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? diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index f848ecedd6b..eabab65c9b0 100644 --- a/app/services/issues/update_service.rb +++ b/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 diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index e5c5368f5d6..589fad16165 100644 --- a/app/services/merge_requests/update_service.rb +++ b/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 diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb index 82e8a83bb3d..2d6fe003215 100644 --- a/spec/models/concerns/mentionable_spec.rb +++ b/spec/models/concerns/mentionable_spec.rb @@ -29,7 +29,7 @@ describe Issue, "Mentionable" do end end - describe '#notice_added_references' do + describe '#create_new_cross_references!' do let(:project) { create(:project) } let(:issues) { create_list(:issue, 2, project: project) } @@ -40,7 +40,7 @@ describe Issue, "Mentionable" do expect(SystemNoteService).not_to receive(:cross_reference) issue.description = 'New description' - issue.notice_added_references + issue.create_new_cross_references! end it 'notifies new references' do @@ -49,7 +49,7 @@ describe Issue, "Mentionable" do expect(SystemNoteService).to receive(:cross_reference).with(issues[1], any_args) issue.description = issues[1].to_reference - issue.notice_added_references + issue.create_new_cross_references! end end @@ -60,7 +60,7 @@ describe Issue, "Mentionable" do expect(SystemNoteService).not_to receive(:cross_reference) issue.update_attributes(description: 'New description') - issue.notice_added_references + issue.create_new_cross_references! end it 'notifies new references' do @@ -69,7 +69,7 @@ describe Issue, "Mentionable" do expect(SystemNoteService).to receive(:cross_reference).with(issues[1], any_args) issue.update_attributes(description: issues[1].to_reference) - issue.notice_added_references + issue.create_new_cross_references! end end diff --git a/spec/support/mentionable_shared_examples.rb b/spec/support/mentionable_shared_examples.rb index a2a0b6905f9..f0717e61781 100644 --- a/spec/support/mentionable_shared_examples.rb +++ b/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 -- cgit v1.2.1