Skip to content

G
gitlab-ce
Commit 1a1e42ad authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets
Browse files
Options

Merge branch 'rs-system-note' into 'master' 

Add SystemNoteService class

The Note model was basically two models crammed together - one handling user-created notes
(i.e., comments on things) and one handling system-created notes (i.e., references).
This splits out the system-specific stuff to a new SystemNoteService class.

See merge request !595
parents ff13fb0a 54db527b
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 754 additions and 729 deletions
app/models/concerns/mentionable.rb
View file @ 1a1e42ad
......@@ -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)
......
app/models/note.rb
View file @ 1a1e42ad
......@@ -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?
system && 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?
......@@ -449,16 +246,6 @@ class Note < ActiveRecord::Base
@discussion_id ||= Note.build_discussion_id(noteable_type, noteable_id || commit_id, line_code)
end
# Returns true if this is a downvote note,
# otherwise false is returned
def downvote?
votable? && (note.start_with?('-1') ||
note.start_with?(':-1:') ||
note.start_with?(':thumbsdown:') ||
note.start_with?(':thumbs_down_sign:')
)
end
def for_commit?
noteable_type == "Commit"
end
......@@ -500,14 +287,18 @@ class Note < ActiveRecord::Base
nil
end
# Returns true if this is an upvote note,
# otherwise false is returned
DOWNVOTES = %w(-1 :-1: :thumbsdown: :thumbs_down_sign:)
# Check if the note is a downvote
def downvote?
votable? && note.start_with?(*DOWNVOTES)
end
UPVOTES = %w(+1 :+1: :thumbsup: :thumbs_up_sign:)
# Check if the note is an upvote
def upvote?
votable? && (note.start_with?('+1') ||
note.start_with?(':+1:') ||
note.start_with?(':thumbsup:') ||
note.start_with?(':thumbs_up_sign:')
)
votable? && note.start_with?(*UPVOTES)
end
def superceded?(notes)
......
app/services/issuable_base_service.rb
View file @ 1a1e42ad
......@@ -2,17 +2,17 @@ class IssuableBaseService < BaseService
private
def create_assignee_note(issuable)
Note.create_assignee_change_note(
SystemNoteService.change_assignee(
issuable, issuable.project, current_user, issuable.assignee)
end
def create_milestone_note(issuable)
Note.create_milestone_change_note(
SystemNoteService.change_milestone(
issuable, issuable.project, current_user, issuable.milestone)
end
def create_labels_note(issuable, added_labels, removed_labels)
Note.create_labels_change_note(
SystemNoteService.change_label(
issuable, issuable.project, current_user, added_labels, removed_labels)
end
end
app/services/issues/close_service.rb
View file @ 1a1e42ad
......@@ -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.change_status(issue, issue.project, current_user, issue.state, current_commit)
end
end
end
app/services/issues/reopen_service.rb
View file @ 1a1e42ad
......@@ -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.change_status(issue, issue.project, current_user, issue.state, nil)
end
end
end
app/services/merge_requests/base_service.rb
View file @ 1a1e42ad
......@@ -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.change_status(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
end
def hook_data(merge_request, action)
......
app/services/merge_requests/refresh_service.rb
View file @ 1a1e42ad
......@@ -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.add_commits(merge_request, merge_request.project,
@current_user, new_commits,
existing_commits, @oldrev)
end
end
......
app/services/system_note_service.rb 0 → 100644
View file @ 1a1e42ad
# 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.)
class SystemNoteService
# 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.add_commits(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 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.change_assignee(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 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.change_label(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.change_milestone(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 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.change_status(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
# 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
def self.cross_reference?(note_text)
note_text.start_with?(cross_reference_note_prefix)
end
# Check if a cross-reference is disallowed
#
# This method prevents adding a "mentioned in !1" note on every single commit
# in a merge request.
#
# noteable - Noteable object being referenced
# mentioner - Mentionable object
#
# Returns Boolean
def self.cross_reference_disallowed?(noteable, mentioner)
return false unless MergeRequest === mentioner
return false unless Commit === noteable
mentioner.commits.include?(noteable)
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: cross_reference_note_content(gfm_reference))
notes.count > 0
end
private
def self.create_note(args = {})
Note.create(args.merge(system: true))
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)
# FIXME (rspeicher): This was breaking things.
# 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_prefix
'mentioned in '
end
def self.cross_reference_note_content(gfm_reference)
"#{cross_reference_note_prefix}#{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
db/migrate/20150509180749_convert_legacy_reference_notes.rb 0 → 100644
View file @ 1a1e42ad
# Convert legacy Markdown-emphasized notes to the current, non-emphasized format
#
# _mentioned in 54f7727c850972f0401c1312a7c4a6a380de5666_
#
# becomes
#
# mentioned in 54f7727c850972f0401c1312a7c4a6a380de5666
class ConvertLegacyReferenceNotes < ActiveRecord::Migration
def up
execute %q{UPDATE notes SET note = trim(both '_' from note) WHERE system = true AND note LIKE '\_%\_'}
end
def down
# noop
end
end
db/schema.rb
View file @ 1a1e42ad
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20150502064022) do
ActiveRecord::Schema.define(version: 20150509180749) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......
spec/factories/notes.rb
View file @ 1a1e42ad
......@@ -25,15 +25,16 @@ FactoryGirl.define do
note "Note"
author
factory :note_on_commit, traits: [:on_commit]
factory :note_on_commit_diff, traits: [:on_commit, :on_diff]
factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
factory :note_on_merge_request, traits: [:on_merge_request]
factory :note_on_commit, traits: [:on_commit]
factory :note_on_commit_diff, traits: [:on_commit, :on_diff]
factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
factory :note_on_merge_request, traits: [:on_merge_request]
factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff]
factory :note_on_project_snippet, traits: [:on_project_snippet]
factory :note_on_project_snippet, traits: [:on_project_snippet]
factory :system_note, traits: [:system]
trait :on_commit do
project factory: :project
project
commit_id RepoHelpers.sample_commit.id
noteable_type "Commit"
end
......@@ -43,7 +44,7 @@ FactoryGirl.define do
end
trait :on_merge_request do
project factory: :project
project
noteable_id 1
noteable_type "MergeRequest"
end
......@@ -58,6 +59,10 @@ FactoryGirl.define do
noteable_type "Snippet"
end
trait :system do
system true
end
trait :with_attachment do
attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png") }
end
......
spec/models/note_spec.rb
View file @ 1a1e42ad
This diff is collapsed.
spec/services/system_note_service_spec.rb 0 → 100644
View file @ 1a1e42ad
This diff is collapsed.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7