diff options
author | Robert Speicher <rspeicher@gmail.com> | 2015-05-01 06:16:19 +0300 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2015-05-11 07:01:01 +0300 |
commit | 48e6fb532a6655b98319403c5e8699f7daf0305e (patch) | |
tree | 9137cd844f66842044bd72705268b44d84d44b6c /app | |
parent | 0e89ff0fb05973bb3ff6930906d52f10517efa62 (diff) |
Add a SystemNoteService class
There's a lot of code in the Note model that only deals with creating
system notes, so we're going to split that into its own class.
Diffstat (limited to 'app')
-rw-r--r-- | app/models/concerns/mentionable.rb | 2 | ||||
-rw-r--r-- | app/models/note.rb | 215 | ||||
-rw-r--r-- | app/services/issuable_base_service.rb | 6 | ||||
-rw-r--r-- | app/services/issues/close_service.rb | 2 | ||||
-rw-r--r-- | app/services/issues/reopen_service.rb | 2 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 2 | ||||
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 5 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 304 |
8 files changed, 320 insertions, 218 deletions
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 3ef3e8b67d8..a5957391bb7 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -39,7 +39,7 @@ module Mentionable # Determine whether or not a cross-reference Note has already been created between this Mentionable and # the specified target. def has_mentioned?(target) - Note.cross_reference_exists?(target, local_reference) + SystemNoteService.cross_reference_exists?(target, local_reference) end def mentioned_users(current_user = nil, p = project) diff --git a/app/models/note.rb b/app/models/note.rb index b32c32e2a20..f2470e5f399 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -63,143 +63,9 @@ class Note < ActiveRecord::Base after_update :set_references class << self - def create_status_change_note(noteable, project, author, status, source) - body = "Status changed to #{status}#{' by ' + source.gfm_reference if source}" - - create( - noteable: noteable, - project: project, - author: author, - note: body, - system: true - ) - end - - # +noteable+ was referenced from +mentioner+, by including GFM in either - # +mentioner+'s description or an associated Note. - # Create a system Note associated with +noteable+ with a GFM back-reference - # to +mentioner+. - def create_cross_reference_note(noteable, mentioner, author) - gfm_reference = mentioner_gfm_ref(noteable, mentioner) - - note_options = { - project: noteable.project, - author: author, - note: cross_reference_note_content(gfm_reference), - system: true - } - - if noteable.kind_of?(Commit) - note_options.merge!(noteable_type: 'Commit', commit_id: noteable.id) - else - note_options.merge!(noteable: noteable) - end - - create(note_options) unless cross_reference_disallowed?(noteable, mentioner) - end - - def create_milestone_change_note(noteable, project, author, milestone) - body = if milestone.nil? - 'Milestone removed' - else - "Milestone changed to #{milestone.title}" - end - - create( - noteable: noteable, - project: project, - author: author, - note: body, - system: true - ) - end - - def create_assignee_change_note(noteable, project, author, assignee) - body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}" - - create({ - noteable: noteable, - project: project, - author: author, - note: body, - system: true - }) - end - - def create_labels_change_note(noteable, project, author, added_labels, removed_labels) - labels_count = added_labels.count + removed_labels.count - added_labels = added_labels.map{ |label| "~#{label.id}" }.join(' ') - removed_labels = removed_labels.map{ |label| "~#{label.id}" }.join(' ') - message = '' - - if added_labels.present? - message << "added #{added_labels}" - end - - if added_labels.present? && removed_labels.present? - message << ' and ' - end - - if removed_labels.present? - message << "removed #{removed_labels}" - end - - message << ' ' << 'label'.pluralize(labels_count) - body = "#{message.capitalize}" - - create( - noteable: noteable, - project: project, - author: author, - note: body, - system: true - ) - end - - def create_new_commits_note(merge_request, project, author, new_commits, existing_commits = [], oldrev = nil) - total_count = new_commits.length + existing_commits.length - commits_text = ActionController::Base.helpers.pluralize(total_count, 'commit') - body = "Added #{commits_text}:\n\n" - - if existing_commits.length > 0 - commit_ids = - if existing_commits.length == 1 - existing_commits.first.short_id - else - if oldrev - "#{Commit.truncate_sha(oldrev)}...#{existing_commits.last.short_id}" - else - "#{existing_commits.first.short_id}..#{existing_commits.last.short_id}" - end - end - - commits_text = ActionController::Base.helpers.pluralize(existing_commits.length, 'commit') - - branch = - if merge_request.for_fork? - "#{merge_request.target_project_namespace}:#{merge_request.target_branch}" - else - merge_request.target_branch - end - - message = "* #{commit_ids} - #{commits_text} from branch `#{branch}`" - body << message - body << "\n" - end - - new_commits.each do |commit| - message = "* #{commit.short_id} - #{commit.title}" - body << message - body << "\n" - end - - create( - noteable: merge_request, - project: project, - author: author, - note: body, - system: true - ) + # TODO (rspeicher): Update usages + def create_cross_reference_note(*args) + SystemNoteService.cross_reference(*args) end def discussions_from_notes(notes) @@ -227,88 +93,19 @@ class Note < ActiveRecord::Base [:discussion, type.try(:underscore), id, line_code].join("-").to_sym end - # Determine if cross reference note should be created. - # eg. mentioning a commit in MR comments which exists inside a MR - # should not create "mentioned in" note. - def cross_reference_disallowed?(noteable, mentioner) - if mentioner.kind_of?(MergeRequest) - mentioner.commits.map(&:id).include? noteable.id - end - end - - # Determine whether or not a cross-reference note already exists. - def cross_reference_exists?(noteable, mentioner) - gfm_reference = mentioner_gfm_ref(noteable, mentioner, true) - notes = if noteable.is_a?(Commit) - where(commit_id: noteable.id, noteable_type: 'Commit') - else - where(noteable_id: noteable.id, noteable_type: noteable.class) - end - - notes.where('note like ?', cross_reference_note_pattern(gfm_reference)). - system.any? - end - def search(query) where("note like :query", query: "%#{query}%") end + end - def cross_reference_note_prefix - 'mentioned in ' - end - - private - - def cross_reference_note_content(gfm_reference) - cross_reference_note_prefix + "#{gfm_reference}" - end - - def cross_reference_note_pattern(gfm_reference) - # Older cross reference notes contained underscores for emphasis - "%" + cross_reference_note_content(gfm_reference) + "%" - end - - # Prepend the mentioner's namespaced project path to the GFM reference for - # cross-project references. For same-project references, return the - # unmodified GFM reference. - def mentioner_gfm_ref(noteable, mentioner, cross_reference = false) - if mentioner.is_a?(Commit) && cross_reference - return mentioner.gfm_reference.sub('commit ', 'commit %') - end - - full_gfm_reference(mentioner.project, noteable.project, mentioner) - end - - # Return the +mentioner+ GFM reference. If the mentioner and noteable - # projects are not the same, add the mentioning project's path to the - # returned value. - def full_gfm_reference(mentioning_project, noteable_project, mentioner) - if mentioning_project == noteable_project - mentioner.gfm_reference - else - if mentioner.is_a?(Commit) - mentioner.gfm_reference.sub( - /(commit )/, - "\\1#{mentioning_project.path_with_namespace}@" - ) - else - mentioner.gfm_reference.sub( - /(issue |merge request )/, - "\\1#{mentioning_project.path_with_namespace}" - ) - end - end - end + def cross_reference? + SystemNoteService.cross_reference?(note) end def max_attachment_size current_application_settings.max_attachment_size.megabytes.to_i end - def cross_reference? - note.start_with?(self.class.cross_reference_note_prefix) - end - def find_diff return nil unless noteable && noteable.diffs.present? diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 5e1906ad2ae..4d251cb30db 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -2,17 +2,17 @@ class IssuableBaseService < BaseService private def create_assignee_note(issuable) - Note.create_assignee_change_note( + SystemNoteService.assignee_change( issuable, issuable.project, current_user, issuable.assignee) end def create_milestone_note(issuable) - Note.create_milestone_change_note( + SystemNoteService.milestone_change( issuable, issuable.project, current_user, issuable.milestone) end def create_labels_note(issuable, added_labels, removed_labels) - Note.create_labels_change_note( + SystemNoteService.label_change( issuable, issuable.project, current_user, added_labels, removed_labels) end end diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index f670019cc63..bdc13685fd0 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -14,7 +14,7 @@ module Issues private def create_note(issue, current_commit) - Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit) + SystemNoteService.status_change(issue, issue.project, current_user, issue.state, current_commit) end end end diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb index 1e5c398516d..fbbf573e5a3 100644 --- a/app/services/issues/reopen_service.rb +++ b/app/services/issues/reopen_service.rb @@ -14,7 +14,7 @@ module Issues private def create_note(issue) - Note.create_status_change_note(issue, issue.project, current_user, issue.state, nil) + SystemNoteService.status_change(issue, issue.project, current_user, issue.state, nil) end end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index f6e1ae6f283..9b470880adf 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -2,7 +2,7 @@ module MergeRequests class BaseService < ::IssuableBaseService def create_note(merge_request) - Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil) + SystemNoteService.status_change(merge_request, merge_request.target_project, current_user, merge_request.state, nil) end def hook_data(merge_request, action) diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index e9b526d1fb7..1d57fd11de7 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -82,8 +82,9 @@ module MergeRequests mr_commit_ids.include?(commit.id) end - Note.create_new_commits_note(merge_request, merge_request.project, - @current_user, new_commits, existing_commits, @oldrev) + SystemNoteService.commit_add(merge_request, merge_request.project, + @current_user, new_commits, + existing_commits, @oldrev) end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb new file mode 100644 index 00000000000..2486360b261 --- /dev/null +++ b/app/services/system_note_service.rb @@ -0,0 +1,304 @@ +# SystemNoteService +# +# Used for creating system notes (e.g., when a user references a merge request +# from an issue, an issue's assignee changes, an issue is closed, etc.) +# +# All methods creating notes should be named using a singular noun and +# present-tense verb (e.g., "assignee_change" not "assignee_changed", +# "label_change" not "labels_change"). +class SystemNoteService + # Called when the assignee of a Noteable is changed or removed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # assignee - User being assigned, or nil + # + # Example Note text: + # + # "Assignee removed" + # + # "Reassigned to @rspeicher" + # + # Returns the created Note object + def self.assignee_change(noteable, project, author, assignee) + body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when a Mentionable references a Noteable + # + # noteable - Noteable object being referenced + # mentioner - Mentionable object + # author - User performing the reference + # + # Example Note text: + # + # "Mentioned in #1" + # + # "Mentioned in !2" + # + # "Mentioned in 54f7727c" + # + # See cross_reference_note_content. + # + # Returns the created Note object + def self.cross_reference(noteable, mentioner, author) + return if cross_reference_disallowed?(noteable, mentioner) + + gfm_reference = mentioner_gfm_ref(noteable, mentioner) + + note_options = { + project: noteable.project, + author: author, + note: cross_reference_note_content(gfm_reference) + } + + if noteable.kind_of?(Commit) + note_options.merge!(noteable_type: 'Commit', commit_id: noteable.id) + else + note_options.merge!(noteable: noteable) + end + + create_note(note_options) + end + + # Called when one or more labels on a Noteable are added and/or removed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # added_labels - Array of Labels added + # removed_labels - Array of Labels removed + # + # Example Note text: + # + # "Added ~1 and removed ~2 ~3 labels" + # + # "Added ~4 label" + # + # "Removed ~5 label" + # + # Returns the created Note object + def self.label_change(noteable, project, author, added_labels, removed_labels) + labels_count = added_labels.count + removed_labels.count + + references = ->(label) { "~#{label.id}" } + added_labels = added_labels.map(&references).join(' ') + removed_labels = removed_labels.map(&references).join(' ') + + body = '' + + if added_labels.present? + body << "added #{added_labels}" + body << ' and ' if removed_labels.present? + end + + if removed_labels.present? + body << "removed #{removed_labels}" + end + + body << ' ' << 'label'.pluralize(labels_count) + body = "#{body.capitalize}" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when the milestone of a Noteable is changed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # milestone - Milestone being assigned, or nil + # + # Example Note text: + # + # "Milestone removed" + # + # "Miletone changed to 7.11" + # + # Returns the created Note object + def self.milestone_change(noteable, project, author, milestone) + body = 'Milestone ' + body += milestone.nil? ? 'removed' : "changed to #{milestone.title}" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when commits are added to a Merge Request + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # new_commits - Array of Commits added since last push + # existing_commits - Array of Commits added in a previous push + # oldrev - TODO (rspeicher): I have no idea what this actually does + # + # See new_commit_summary and existing_commit_summary. + # + # Returns the created Note object + def self.commit_add(noteable, project, author, new_commits, existing_commits = [], oldrev = nil) + total_count = new_commits.length + existing_commits.length + commits_text = "#{total_count} commit".pluralize(total_count) + + body = "Added #{commits_text}:\n\n" + body << existing_commit_summary(noteable, existing_commits, oldrev) + body << new_commit_summary(new_commits).join("\n") + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when the status of a Noteable is changed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # status - String status + # source - Mentionable performing the change, or nil + # + # Example Note text: + # + # "Status changed to merged" + # + # "Status changed to closed by bc17db76" + # + # Returns the created Note object + def self.status_change(noteable, project, author, status, source) + body = "Status changed to #{status}" + body += " by #{source.gfm_reference}" if source + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + def self.cross_reference?(note_text) + note_text.start_with?(cross_reference_note_prefix) + end + + # Determine if cross reference note should be created. + # eg. mentioning a commit in MR comments which exists inside a MR + # should not create "mentioned in" note. + def self.cross_reference_disallowed?(noteable, mentioner) + if mentioner.kind_of?(MergeRequest) + mentioner.commits.map(&:id).include? noteable.id + end + end + + def self.cross_reference_exists?(noteable, mentioner) + # Initial scope should be system notes of this noteable type + notes = Note.system.where(noteable_type: noteable.class) + + if noteable.is_a?(Commit) + # Commits have non-integer IDs, so they're stored in `commit_id` + notes = notes.where(commit_id: noteable.id) + else + notes = notes.where(noteable_id: noteable.id) + end + + gfm_reference = mentioner_gfm_ref(noteable, mentioner, true) + notes = notes.where('note like ?', cross_reference_note_pattern(gfm_reference)) + + notes.count > 0 + end + + private + + def self.create_note(args = {}) + Note.create(args.merge(system: true)) + end + + def self.cross_reference_note_prefix + 'mentioned in ' + end + + # Prepend the mentioner's namespaced project path to the GFM reference for + # cross-project references. For same-project references, return the + # unmodified GFM reference. + def self.mentioner_gfm_ref(noteable, mentioner, cross_reference = false) + if mentioner.is_a?(Commit) && cross_reference + return mentioner.gfm_reference.sub('commit ', 'commit %') + end + + full_gfm_reference(mentioner.project, noteable.project, mentioner) + end + + # Return the +mentioner+ GFM reference. If the mentioner and noteable + # projects are not the same, add the mentioning project's path to the + # returned value. + def self.full_gfm_reference(mentioning_project, noteable_project, mentioner) + if mentioning_project == noteable_project + mentioner.gfm_reference + else + if mentioner.is_a?(Commit) + mentioner.gfm_reference.sub( + /(commit )/, + "\\1#{mentioning_project.path_with_namespace}@" + ) + else + mentioner.gfm_reference.sub( + /(issue |merge request )/, + "\\1#{mentioning_project.path_with_namespace}" + ) + end + end + end + + def self.cross_reference_note_content(gfm_reference) + cross_reference_note_prefix + "#{gfm_reference}" + end + + def self.cross_reference_note_pattern(gfm_reference) + # Older cross reference notes contained underscores for emphasis + "%" + cross_reference_note_content(gfm_reference) + "%" + end + + # Build an Array of lines detailing each commit added in a merge request + # + # new_commits - Array of new Commit objects + # + # Returns an Array of Strings + def self.new_commit_summary(new_commits) + new_commits.collect do |commit| + "* #{commit.short_id} - #{commit.title}" + end + end + + # Build a single line summarizing existing commits being added in a merge + # request + # + # noteable - MergeRequest object + # existing_commits - Array of existing Commit objects + # oldrev - Optional String SHA of ... TODO (rspeicher): I have no idea what this actually does. + # + # Examples: + # + # "* ea0f8418...2f4426b7 - 24 commits from branch `master`" + # + # "* ea0f8418..4188f0ea - 15 commits from branch `fork:master`" + # + # "* ea0f8418 - 1 commit from branch `feature`" + # + # Returns a newline-terminated String + def self.existing_commit_summary(noteable, existing_commits, oldrev = nil) + return '' if existing_commits.empty? + + count = existing_commits.size + + commit_ids = if count == 1 + existing_commits.first.short_id + else + if oldrev + "#{Commit.truncate_sha(oldrev)}...#{existing_commits.last.short_id}" + else + "#{existing_commits.first.short_id}..#{existing_commits.last.short_id}" + end + end + + commits_text = "#{count} commit".pluralize(count) + + branch = noteable.target_branch + branch = "#{noteable.target_project_namespace}:#{branch}" if noteable.for_fork? + + "* #{commit_ids} - #{commits_text} from branch `#{branch}`\n" + end +end |