diff options
author | Robert Speicher <robert@gitlab.com> | 2015-10-14 23:10:34 +0300 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2015-10-14 23:10:34 +0300 |
commit | 123669a55107514798ba531ba3a744b3ec8503ee (patch) | |
tree | d44c8f0dbd7f0426341392c755af60d023a3d808 | |
parent | a3a80eac110fb9f195eaccd3afd8e8611d1924dc (diff) | |
parent | 276b3a7bc202bd9b51c8f5401f4c525227f3a4d8 (diff) |
Merge branch 'simplify-cross-references' into 'master'
Simplify code around (cross)-references
See merge request !1568
-rw-r--r-- | app/controllers/projects/issues_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 2 | ||||
-rw-r--r-- | app/models/concerns/mentionable.rb | 44 | ||||
-rw-r--r-- | app/models/concerns/participable.rb | 22 | ||||
-rw-r--r-- | app/models/note.rb | 13 | ||||
-rw-r--r-- | app/services/git_push_service.rb | 50 | ||||
-rw-r--r-- | app/services/issues/create_service.rb | 2 | ||||
-rw-r--r-- | app/services/issues/update_service.rb | 2 | ||||
-rw-r--r-- | app/services/merge_requests/create_service.rb | 2 | ||||
-rw-r--r-- | app/services/merge_requests/update_service.rb | 2 | ||||
-rw-r--r-- | app/services/notes/create_service.rb | 8 | ||||
-rw-r--r-- | app/services/notes/update_service.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/reference_extractor.rb | 2 | ||||
-rw-r--r-- | spec/models/commit_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/concerns/mentionable_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/issue_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/note_spec.rb | 5 | ||||
-rw-r--r-- | spec/services/git_push_service_spec.rb | 2 | ||||
-rw-r--r-- | spec/support/mentionable_shared_examples.rb | 15 |
19 files changed, 70 insertions, 113 deletions
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 0f89f2e88cc..4612abcbae8 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -55,7 +55,7 @@ class Projects::IssuesController < Projects::ApplicationController end def show - @participants = @issue.participants(current_user, @project) + @participants = @issue.participants(current_user) @note = @project.notes.new(noteable: @issue) @notes = @issue.notes.inc_author.fresh @noteable = @issue diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 7570934e727..98df6984bf7 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -246,7 +246,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def define_show_vars - @participants = @merge_request.participants(current_user, @project) + @participants = @merge_request.participants(current_user) # Build a note object for comment form @note = @project.notes.new(noteable: @merge_request) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 5b0ae411642..b34def66d2e 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -41,55 +41,49 @@ module Mentionable self end - # Determine whether or not a cross-reference Note has already been created between this Mentionable and - # the specified target. - def has_mentioned?(target) - SystemNoteService.cross_reference_exists?(target, local_reference) + def all_references(current_user = self.author, text = self.mentionable_text) + ext = Gitlab::ReferenceExtractor.new(self.project, current_user) + ext.analyze(text) + ext end def mentioned_users(current_user = nil) - return [] if mentionable_text.blank? - - ext = Gitlab::ReferenceExtractor.new(self.project, current_user) - ext.analyze(mentionable_text) - ext.users.uniq + all_references(current_user).users.uniq end # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. - def references(p = project, current_user = self.author, text = mentionable_text) + def referenced_mentionables(current_user = self.author, text = self.mentionable_text) return [] if text.blank? - ext = Gitlab::ReferenceExtractor.new(p, current_user) - ext.analyze(text) - - (ext.issues + ext.merge_requests + ext.commits).uniq - [local_reference] + refs = all_references(current_user, text) + (refs.issues + refs.merge_requests + refs.commits).uniq - [local_reference] end # Create a cross-reference Note for each GFM reference to another Mentionable found in +mentionable_text+. - def create_cross_references!(p = project, a = author, without = []) - refs = references(p) - + def create_cross_references!(author = self.author, without = [], text = self.mentionable_text) + refs = referenced_mentionables(author, text) + # We're using this method instead of Array diffing because that requires # both of the object's `hash` values to be the same, which may not be the # case for otherwise identical Commit objects. - refs.reject! { |ref| without.include?(ref) } + refs.reject! { |ref| without.include?(ref) || cross_reference_exists?(ref) } refs.each do |ref| - SystemNoteService.cross_reference(ref, local_reference, a) + SystemNoteService.cross_reference(ref, local_reference, author) 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) + def create_new_cross_references!(author = self.author) changes = detect_mentionable_changes return if changes.empty? original_text = changes.collect { |_, vals| vals.first }.join(' ') - preexisting = references(p, self.author, original_text) - create_cross_references!(p, a, preexisting) + preexisting = referenced_mentionables(author, original_text) + create_cross_references!(author, preexisting) end private @@ -111,4 +105,10 @@ module Mentionable # Only include changed fields that are mentionable source.select { |key, val| mentionable.include?(key) } 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) + SystemNoteService.cross_reference_exists?(target, local_reference) + end end diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index 7c9597333dd..ffc874357fd 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -37,8 +37,8 @@ module Participable # Be aware that this method makes a lot of sql queries. # Save result into variable if you are going to reuse it inside same request - def participants(current_user = self.author, project = self.project) - participants = self.class.participant_attrs.flat_map do |attr| + def participants(current_user = self.author) + self.class.participant_attrs.flat_map do |attr| meth = method(attr) value = @@ -48,28 +48,22 @@ module Participable meth.call end - participants_for(value, current_user, project) - end.compact.uniq - - if project - participants.select! do |user| - user.can?(:read_project, project) - end + participants_for(value, current_user) + end.compact.uniq.select do |user| + user.can?(:read_project, self.project) end - - participants end private - def participants_for(value, current_user = nil, project = nil) + def participants_for(value, current_user = nil) case value when User [value] when Enumerable, ActiveRecord::Relation - value.flat_map { |v| participants_for(v, current_user, project) } + value.flat_map { |v| participants_for(v, current_user) } when Participable - value.participants(current_user, project) + value.participants(current_user) end end end diff --git a/app/models/note.rb b/app/models/note.rb index de3b6df88f7..ee0c14598f3 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -62,7 +62,6 @@ class Note < ActiveRecord::Base serialize :st_diff before_create :set_diff, if: ->(n) { n.line_code.present? } - after_update :set_references class << self def discussions_from_notes(notes) @@ -333,15 +332,13 @@ class Note < ActiveRecord::Base end def noteable_type_name - if noteable_type.present? - noteable_type.downcase - end + noteable_type.downcase if noteable_type.present? end # FIXME: Hack for polymorphic associations with STI # For more information visit http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#label-Polymorphic+Associations - def noteable_type=(sType) - super(sType.to_s.classify.constantize.base_class.to_s) + def noteable_type=(noteable_type) + super(noteable_type.to_s.classify.constantize.base_class.to_s) end # Reset notes events cache @@ -357,10 +354,6 @@ class Note < ActiveRecord::Base Event.reset_event_cache_for(self) end - def set_references - create_new_cross_references!(project, author) - end - def system? read_attribute(:system) end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index f9a8265d2d4..81d47602f13 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -74,48 +74,30 @@ class GitPushService def process_commit_messages(ref) is_default_branch = is_default_branch?(ref) - @push_commits.each do |commit| - # Close issues if these commits were pushed to the project's default branch and the commit message matches the - # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to - # a different branch. - issues_to_close = commit.closes_issues(user) + authors = Hash.new do |hash, commit| + email = commit.author_email + return hash[email] if hash.has_key?(email) - # Load commit author only if needed. - # For push with 1k commits it prevents 900+ requests in database - author = nil + hash[email] = commit_user(commit) + end + @push_commits.each do |commit| # Keep track of the issues that will be actually closed because they are on a default branch. # Hence, when creating cross-reference notes, the not-closed issues (on non-default branches) # will also have cross-reference. - actually_closed_issues = [] - - if issues_to_close.present? && is_default_branch - author ||= commit_user(commit) - actually_closed_issues = issues_to_close - issues_to_close.each do |issue| - Issues::CloseService.new(project, author, {}).execute(issue, commit) + closed_issues = [] + + if is_default_branch + # Close issues if these commits were pushed to the project's default branch and the commit message matches the + # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to + # a different branch. + closed_issues = commit.closes_issues(user) + closed_issues.each do |issue| + Issues::CloseService.new(project, authors[commit], {}).execute(issue, commit) end end - if project.default_issues_tracker? - create_cross_reference_notes(commit, actually_closed_issues) - end - end - end - - def create_cross_reference_notes(commit, issues_to_close) - # Create cross-reference notes for any other references than those given in issues_to_close. - # Omit any issues that were referenced in an issue-closing phrase, or have already been - # mentioned from this commit (probably from this commit being pushed to a different branch). - refs = commit.references(project, user) - issues_to_close - refs.reject! { |r| commit.has_mentioned?(r) } - - if refs.present? - author ||= commit_user(commit) - - refs.each do |r| - SystemNoteService.cross_reference(r, commit, author) - end + commit.create_cross_references!(authors[commit], closed_issues) end end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 1ea4b72216c..bcb380d3215 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -10,7 +10,7 @@ module Issues issue.update_attributes(label_ids: label_params) notification_service.new_issue(issue, current_user) event_service.open_issue(issue, current_user) - issue.create_cross_references!(issue.project, current_user) + issue.create_cross_references!(current_user) execute_hooks(issue, 'open') end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 2fc6ef7f356..2b5426ad452 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.create_new_cross_references!(issue.project, current_user) + issue.create_new_cross_references! execute_hooks(issue, 'update') end diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 9651b16462c..009d5a6867e 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -18,7 +18,7 @@ module MergeRequests merge_request.update_attributes(label_ids: label_params) event_service.open_mr(merge_request, current_user) notification_service.new_merge_request(merge_request, current_user) - merge_request.create_cross_references!(merge_request.project, current_user) + merge_request.create_cross_references!(current_user) execute_hooks(merge_request) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 25d79e22e39..ebbe0af803b 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.create_new_cross_references!(merge_request.project, current_user) + merge_request.create_new_cross_references! execute_hooks(merge_request, 'update') end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 482c0444049..2001dc89c33 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -11,13 +11,7 @@ module Notes # Skip system notes, like status changes and cross-references. unless note.system event_service.leave_note(note, note.author) - - # Create a cross-reference note if this Note contains GFM that names an - # issue, merge request, or commit. - note.references.each do |mentioned| - SystemNoteService.cross_reference(mentioned, note.noteable, note.author) - end - + note.create_cross_references! execute_hooks(note) end end diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index c22a9333ef6..6c2f08e5963 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -4,7 +4,7 @@ module Notes return note unless note.editable? note.update_attributes(params.merge(updated_by: current_user)) - + note.create_new_cross_references! note.reset_events_cache note diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 0961bd80421..30497e274c2 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -39,6 +39,8 @@ module Gitlab # # Returns the results Array for the requested filter type def pipeline_result(filter_type) + return [] if @text.blank? + klass = filter_type.to_s.camelize + 'ReferenceFilter' filter = Gitlab::Markdown.const_get(klass) diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index e303a97e6b5..90be9324951 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -89,9 +89,9 @@ eos end it_behaves_like 'a mentionable' do - subject { commit } + subject { create(:project).commit } - let(:author) { create(:user, email: commit.author_email) } + let(:author) { create(:user, email: subject.author_email) } let(:backref_text) { "commit #{subject.id}" } let(:set_mentionable_text) do ->(txt) { allow(subject).to receive(:safe_message).and_return(txt) } diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb index 2d6fe003215..6179882e935 100644 --- a/spec/models/concerns/mentionable_spec.rb +++ b/spec/models/concerns/mentionable_spec.rb @@ -25,7 +25,7 @@ describe Issue, "Mentionable" do it 'correctly removes already-mentioned Commits' do expect(SystemNoteService).not_to receive(:cross_reference) - issue.create_cross_references!(project, author, [commit2]) + issue.create_cross_references!(author, [commit2]) end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index cf336d82957..623332cd2f9 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -69,7 +69,7 @@ describe Issue do end it_behaves_like 'an editable mentionable' do - subject { create(:issue, project: project) } + subject { create(:issue) } let(:backref_text) { "issue #{subject.to_reference}" } let(:set_mentionable_text) { ->(txt){ subject.description = txt } } diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 3a0b194ba1e..75564839dcf 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -192,10 +192,9 @@ describe Note do end it_behaves_like 'an editable mentionable' do - subject { create :note, noteable: issue, project: project } + subject { create :note, noteable: issue, project: issue.project } - let(:project) { create(:project) } - let(:issue) { create :issue, project: project } + let(:issue) { create :issue } let(:backref_text) { issue.gfm_reference } let(:set_mentionable_text) { ->(txt) { subject.note = txt } } end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index c483060fd73..fd72905c331 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -155,7 +155,7 @@ describe GitPushService do before do allow(commit).to receive_messages( - safe_message: "this commit \n mentions ##{issue.id}", + safe_message: "this commit \n mentions #{issue.to_reference}", references: [issue], author_name: commit_author.name, author_email: commit_author.email diff --git a/spec/support/mentionable_shared_examples.rb b/spec/support/mentionable_shared_examples.rb index e3de0afb448..f584904845e 100644 --- a/spec/support/mentionable_shared_examples.rb +++ b/spec/support/mentionable_shared_examples.rb @@ -5,7 +5,7 @@ # - let(:set_mentionable_text) { lambda { |txt| "block that assigns txt to the subject's mentionable_text" } } def common_mentionable_setup - let(:project) { create :project } + let(:project) { subject.project } let(:author) { subject.author } let(:mentioned_issue) { create(:issue, project: project) } @@ -65,7 +65,7 @@ shared_examples 'a mentionable' do it "extracts references from its reference property" do # De-duplicate and omit itself - refs = subject.references(project) + refs = subject.referenced_mentionables expect(refs.size).to eq(6) expect(refs).to include(mentioned_issue) expect(refs).to include(mentioned_mr) @@ -84,14 +84,7 @@ shared_examples 'a mentionable' do with(referenced, subject.local_reference, author) end - subject.create_cross_references!(project, author) - end - - it 'detects existing cross-references' do - SystemNoteService.cross_reference(mentioned_issue, subject.local_reference, author) - - expect(subject).to have_mentioned(mentioned_issue) - expect(subject).not_to have_mentioned(mentioned_mr) + subject.create_cross_references! end end @@ -143,6 +136,6 @@ shared_examples 'an editable mentionable' do end set_mentionable_text.call(new_text) - subject.create_new_cross_references!(project, author) + subject.create_new_cross_references!(author) end end |