Skip to content

G
gitlab-ce
Commit 0c3852b1 authored by Sean McGivern's avatar Sean McGivern
Browse files
Options

Merge branch '17597-extract-commit-system-note-service-pd' into 'master' 

Extract Commit System Notes Service

Closes #17597

See merge request gitlab-org/gitlab!17808
parents 247eeb32 7d0a27a2
Hide whitespace changes
Inline Side-by-side
Showing with 343 additions and 208 deletions
app/services/system_note_service.rb
View file @ 0c3852b1
...@@ -16,20 +16,9 @@ module SystemNoteService ...@@ -16,20 +16,9 @@ module SystemNoteService
# existing_commits - Array of Commits added in a previous push # existing_commits - Array of Commits added in a previous push
# oldrev - Optional String SHA of a previous Commit # oldrev - Optional String SHA of a previous Commit
# #
# See new_commit_summary and existing_commit_summary.
#
# Returns the created Note object # Returns the created Note object
def add_commits(noteable, project, author, new_commits, existing_commits = [], oldrev = nil) def add_commits(noteable, project, author, new_commits, existing_commits = [], oldrev = nil)
total_count = new_commits.length + existing_commits.length ::SystemNotes::CommitService.new(noteable: noteable, project: project, author: author).add_commits(new_commits, existing_commits, oldrev)
commits_text = "#{total_count} commit".pluralize(total_count)
text_parts = ["added #{commits_text}"]
text_parts << commits_list(noteable, new_commits, existing_commits, oldrev)
text_parts << "[Compare with previous version](#{diff_comparison_path(noteable, project, oldrev)})"
body = text_parts.join("\n\n")
create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count))
end end
# Called when a commit was tagged # Called when a commit was tagged
...@@ -41,10 +30,7 @@ module SystemNoteService ...@@ -41,10 +30,7 @@ module SystemNoteService
# #
# Returns the created Note object # Returns the created Note object
def tag_commit(noteable, project, author, tag_name) def tag_commit(noteable, project, author, tag_name)
link = url_helpers.project_tag_path(project, id: tag_name) ::SystemNotes::CommitService.new(noteable: noteable, project: project, author: author).tag_commit(tag_name)
body = "tagged commit #{noteable.sha} to [`#{tag_name}`](#{link})"
create_note(NoteSummary.new(noteable, project, author, body, action: 'tag'))
end end
# Called when the assignee of a Noteable is changed or removed # Called when the assignee of a Noteable is changed or removed
...@@ -497,17 +483,6 @@ module SystemNoteService ...@@ -497,17 +483,6 @@ module SystemNoteService
notes_for_mentioner(mentioner, noteable, notes).exists? notes_for_mentioner(mentioner, noteable, notes).exists?
end 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 new_commit_summary(new_commits)
new_commits.collect do |commit|
content_tag('li', "#{commit.short_id} - #{commit.title}")
end
end
# Called when the status of a Task has changed # Called when the status of a Task has changed
# #
# noteable - Noteable object. # noteable - Noteable object.
...@@ -637,71 +612,10 @@ module SystemNoteService ...@@ -637,71 +612,10 @@ module SystemNoteService
"#{cross_reference_note_prefix}#{gfm_reference}" "#{cross_reference_note_prefix}#{gfm_reference}"
end end
# Builds a list of existing and new commits according to existing_commits and
# new_commits methods.
# Returns a String wrapped in `ul` and `li` tags.
def commits_list(noteable, new_commits, existing_commits, oldrev)
existing_commit_summary = existing_commit_summary(noteable, existing_commits, oldrev)
new_commit_summary = new_commit_summary(new_commits).join
content_tag('ul', "#{existing_commit_summary}#{new_commit_summary}".html_safe)
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 a previous Commit
#
# 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 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 && !Gitlab::Git.blank_ref?(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?
branch_name = content_tag('code', branch)
content_tag('li', "#{commit_ids} - #{commits_text} from branch #{branch_name}".html_safe)
end
def url_helpers def url_helpers
@url_helpers ||= Gitlab::Routing.url_helpers @url_helpers ||= Gitlab::Routing.url_helpers
end end
def diff_comparison_path(merge_request, project, oldrev)
diff_id = merge_request.merge_request_diff.id
url_helpers.diffs_project_merge_request_path(
project,
merge_request,
diff_id: diff_id,
start_sha: oldrev
)
end
def content_tag(*args) def content_tag(*args)
ActionController::Base.helpers.content_tag(*args) ActionController::Base.helpers.content_tag(*args)
end end
......
app/services/system_notes/base_service.rb 0 → 100644
View file @ 0c3852b1
# frozen_string_literal: true
module SystemNotes
class BaseService
attr_reader :noteable, :project, :author
def initialize(noteable: nil, author: nil, project: nil)
@noteable = noteable
@project = project
@author = author
end
protected
def create_note(note_summary)
note = Note.create(note_summary.note.merge(system: true))
note.system_note_metadata = SystemNoteMetadata.new(note_summary.metadata) if note_summary.metadata?
note
end
def content_tag(*args)
ActionController::Base.helpers.content_tag(*args)
end
def url_helpers
@url_helpers ||= Gitlab::Routing.url_helpers
end
end
end
app/services/system_notes/commit_service.rb 0 → 100644
View file @ 0c3852b1
# frozen_string_literal: true
module SystemNotes
class CommitService < ::SystemNotes::BaseService
# Called when commits are added to a Merge Request
#
# new_commits - Array of Commits added since last push
# existing_commits - Array of Commits added in a previous push
# oldrev - Optional String SHA of a previous Commit
#
# See new_commit_summary and existing_commit_summary.
#
# Returns the created Note object
def add_commits(new_commits, existing_commits = [], oldrev = nil)
total_count = new_commits.length + existing_commits.length
commits_text = "#{total_count} commit".pluralize(total_count)
text_parts = ["added #{commits_text}"]
text_parts << commits_list(noteable, new_commits, existing_commits, oldrev)
text_parts << "[Compare with previous version](#{diff_comparison_path(noteable, project, oldrev)})"
body = text_parts.join("\n\n")
create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count))
end
# Called when a commit was tagged
#
# tag_name - The created tag name
#
# Returns the created Note object
def tag_commit(tag_name)
link = url_helpers.project_tag_path(project, id: tag_name)
body = "tagged commit #{noteable.sha} to [`#{tag_name}`](#{link})"
create_note(NoteSummary.new(noteable, project, author, body, action: 'tag'))
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 new_commit_summary(new_commits)
new_commits.collect do |commit|
content_tag('li', "#{commit.short_id} - #{commit.title}")
end
end
private
# Builds a list of existing and new commits according to existing_commits and
# new_commits methods.
# Returns a String wrapped in `ul` and `li` tags.
def commits_list(noteable, new_commits, existing_commits, oldrev)
existing_commit_summary = existing_commit_summary(noteable, existing_commits, oldrev)
new_commit_summary = new_commit_summary(new_commits).join
content_tag('ul', "#{existing_commit_summary}#{new_commit_summary}".html_safe)
end
# Build a single line summarizing existing commits being added in a merge
# request
#
# existing_commits - Array of existing Commit objects
# oldrev - Optional String SHA of a previous Commit
#
# 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 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 && !Gitlab::Git.blank_ref?(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?
branch_name = content_tag('code', branch)
content_tag('li', "#{commit_ids} - #{commits_text} from branch #{branch_name}".html_safe)
end
def diff_comparison_path(merge_request, project, oldrev)
diff_id = merge_request.merge_request_diff.id
url_helpers.diffs_project_merge_request_path(
project,
merge_request,
diff_id: diff_id,
start_sha: oldrev
)
end
end
end
spec/services/system_note_service_spec.rb
View file @ 0c3852b1
...@@ -14,127 +14,29 @@ describe SystemNoteService do ...@@ -14,127 +14,29 @@ describe SystemNoteService do
let(:noteable) { create(:issue, project: project) } let(:noteable) { create(:issue, project: project) }
let(:issue) { noteable } let(:issue) { noteable }
shared_examples_for 'a note with overridable created_at' do
let(:noteable) { create(:issue, project: project, system_note_timestamp: Time.at(42)) }
it 'the note has the correct time' do
expect(subject.created_at).to eq Time.at(42)
end
end
shared_examples_for 'a system note' do
let(:expected_noteable) { noteable }
let(:commit_count) { nil }
it 'has the correct attributes', :aggregate_failures do
expect(subject).to be_valid
expect(subject).to be_system
expect(subject.noteable).to eq expected_noteable
expect(subject.project).to eq project
expect(subject.author).to eq author
expect(subject.system_note_metadata.action).to eq(action)
expect(subject.system_note_metadata.commit_count).to eq(commit_count)
end
end
describe '.add_commits' do describe '.add_commits' do
subject { described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev) } let(:new_commits) { double }
let(:old_commits) { double }
let(:noteable) { create(:merge_request, source_project: project, target_project: project) } let(:oldrev) { double }
let(:new_commits) { noteable.commits }
let(:old_commits) { [] }
let(:oldrev) { nil }
it_behaves_like 'a system note' do
let(:commit_count) { new_commits.size }
let(:action) { 'commit' }
end
describe 'note body' do it 'calls CommitService' do
let(:note_lines) { subject.note.split("\n").reject(&:blank?) } expect_next_instance_of(::SystemNotes::CommitService) do |service|
expect(service).to receive(:add_commits).with(new_commits, old_commits, oldrev)
describe 'comparison diff link line' do
it 'adds the comparison text' do
expect(note_lines[2]).to match "[Compare with previous version]"
end
end end
context 'without existing commits' do described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev)
it 'adds a message header' do
expect(note_lines[0]).to eq "added #{new_commits.size} commits"
end
it 'adds a message for each commit' do
decoded_note_content = HTMLEntities.new.decode(subject.note)
new_commits.each do |commit|
expect(decoded_note_content).to include("<li>#{commit.short_id} - #{commit.title}</li>")
end
end
end
describe 'summary line for existing commits' do
let(:summary_line) { note_lines[1] }
context 'with one existing commit' do
let(:old_commits) { [noteable.commits.last] }
it 'includes the existing commit' do
expect(summary_line).to start_with("<ul><li>#{old_commits.first.short_id} - 1 commit from branch <code>feature</code>")
end
end
context 'with multiple existing commits' do
let(:old_commits) { noteable.commits[3..-1] }
context 'with oldrev' do
let(:oldrev) { noteable.commits[2].id }
it 'includes a commit range and count' do
expect(summary_line)
.to start_with("<ul><li>#{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id} - 26 commits from branch <code>feature</code>")
end
end
context 'without oldrev' do
it 'includes a commit range and count' do
expect(summary_line)
.to start_with("<ul><li>#{old_commits[0].short_id}..#{old_commits[-1].short_id} - 26 commits from branch <code>feature</code>")
end
end
context 'on a fork' do
before do
expect(noteable).to receive(:for_fork?).and_return(true)
end
it 'includes the project namespace' do
expect(summary_line).to include("<code>#{noteable.target_project_namespace}:feature</code>")
end
end
end
end
end end
end end
describe '.tag_commit' do describe '.tag_commit' do
let(:noteable) do let(:tag_name) { double }
project.commit
end
let(:tag_name) { 'v1.2.3' }
subject { described_class.tag_commit(noteable, project, author, tag_name) }
it_behaves_like 'a system note' do
let(:action) { 'tag' }
end
it 'sets the note text' do it 'calls CommitService' do
link = "/#{project.full_path}/-/tags/#{tag_name}" expect_next_instance_of(::SystemNotes::CommitService) do |service|
expect(service).to receive(:tag_commit).with(tag_name)
end
expect(subject.note).to eq "tagged commit #{noteable.sha} to [`#{tag_name}`](#{link})" described_class.tag_commit(noteable, project, author, tag_name)
end end
end end
...@@ -804,15 +706,6 @@ describe SystemNoteService do ...@@ -804,15 +706,6 @@ describe SystemNoteService do
end end
end end
describe '.new_commit_summary' do
it 'escapes HTML titles' do
commit = double(title: '<pre>This is a test</pre>', short_id: '12345678')
escaped = '&lt;pre&gt;This is a test&lt;/pre&gt;'
expect(described_class.new_commit_summary([commit])).to all(match(/- #{escaped}/))
end
end
describe 'Jira integration' do describe 'Jira integration' do
include JiraServiceHelper include JiraServiceHelper
......
spec/services/system_notes/base_service_spec.rb 0 → 100644
View file @ 0c3852b1
# frozen_string_literal: true
require 'spec_helper'
describe SystemNotes::BaseService do
let(:noteable) { double }
let(:project) { double }
let(:author) { double }
let(:base_service) { described_class.new(noteable: noteable, project: project, author: author) }
describe '#noteable' do
subject { base_service.noteable }
it { is_expected.to eq(noteable) }
it 'returns nil if no arguments are given' do
instance = described_class.new
expect(instance.noteable).to be_nil
end
end
describe '#project' do
subject { base_service.project }
it { is_expected.to eq(project) }
it 'returns nil if no arguments are given' do
instance = described_class.new
expect(instance.project).to be_nil
end
end
describe '#author' do
subject { base_service.author }
it { is_expected.to eq(author) }
it 'returns nil if no arguments are given' do
instance = described_class.new
expect(instance.author).to be_nil
end
end
end
spec/services/system_notes/commit_service_spec.rb 0 → 100644
View file @ 0c3852b1
# frozen_string_literal: true
require 'spec_helper'
describe SystemNotes::CommitService do
set(:group) { create(:group) }
set(:project) { create(:project, :repository, group: group) }
set(:author) { create(:user) }
let(:commit_service) { described_class.new(noteable: noteable, project: project, author: author) }
describe '#add_commits' do
subject { commit_service.add_commits(new_commits, old_commits, oldrev) }
let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
let(:new_commits) { noteable.commits }
let(:old_commits) { [] }
let(:oldrev) { nil }
it_behaves_like 'a system note' do
let(:commit_count) { new_commits.size }
let(:action) { 'commit' }
end
describe 'note body' do
let(:note_lines) { subject.note.split("\n").reject(&:blank?) }
describe 'comparison diff link line' do
it 'adds the comparison text' do
expect(note_lines[2]).to match "[Compare with previous version]"
end
end
context 'without existing commits' do
it 'adds a message header' do
expect(note_lines[0]).to eq "added #{new_commits.size} commits"
end
it 'adds a message for each commit' do
decoded_note_content = HTMLEntities.new.decode(subject.note)
new_commits.each do |commit|
expect(decoded_note_content).to include("<li>#{commit.short_id} - #{commit.title}</li>")
end
end
end
describe 'summary line for existing commits' do
let(:summary_line) { note_lines[1] }
context 'with one existing commit' do
let(:old_commits) { [noteable.commits.last] }
it 'includes the existing commit' do
expect(summary_line).to start_with("<ul><li>#{old_commits.first.short_id} - 1 commit from branch <code>feature</code>")
end
end
context 'with multiple existing commits' do
let(:old_commits) { noteable.commits[3..-1] }
context 'with oldrev' do
let(:oldrev) { noteable.commits[2].id }
it 'includes a commit range and count' do
expect(summary_line)
.to start_with("<ul><li>#{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id} - 26 commits from branch <code>feature</code>")
end
end
context 'without oldrev' do
it 'includes a commit range and count' do
expect(summary_line)
.to start_with("<ul><li>#{old_commits[0].short_id}..#{old_commits[-1].short_id} - 26 commits from branch <code>feature</code>")
end
end
context 'on a fork' do
before do
expect(noteable).to receive(:for_fork?).and_return(true)
end
it 'includes the project namespace' do
expect(summary_line).to include("<code>#{noteable.target_project_namespace}:feature</code>")
end
end
end
end
end
end
describe '#tag_commit' do
let(:noteable) { project.commit }
let(:tag_name) { 'v1.2.3' }
subject { commit_service.tag_commit(tag_name) }
it_behaves_like 'a system note' do
let(:action) { 'tag' }
end
it 'sets the note text' do
link = "/#{project.full_path}/-/tags/#{tag_name}"
expect(subject.note).to eq "tagged commit #{noteable.sha} to [`#{tag_name}`](#{link})"
end
end
describe '#new_commit_summary' do
it 'escapes HTML titles' do
commit = double(title: '<pre>This is a test</pre>', short_id: '12345678')
escaped = '&lt;pre&gt;This is a test&lt;/pre&gt;'
expect(described_class.new.new_commit_summary([commit])).to all(match(/- #{escaped}/))
end
end
end
spec/support/shared_examples/common_system_notes_examples.rb
View file @ 0c3852b1
...@@ -27,3 +27,28 @@ shared_examples 'WIP notes creation' do |wip_action| ...@@ -27,3 +27,28 @@ shared_examples 'WIP notes creation' do |wip_action|
expect(Note.second.note).to match('changed title') expect(Note.second.note).to match('changed title')
end end
end end
shared_examples_for 'a note with overridable created_at' do
let(:noteable) { create(:issue, project: project, system_note_timestamp: Time.at(42)) }
it 'the note has the correct time' do
expect(subject.created_at).to eq Time.at(42)
end
end
shared_examples_for 'a system note' do
let(:expected_noteable) { noteable }
let(:commit_count) { nil }
it 'has the correct attributes', :aggregate_failures do
expect(subject).to be_valid
expect(subject).to be_system
expect(subject.noteable).to eq expected_noteable
expect(subject.project).to eq project
expect(subject.author).to eq author
expect(subject.system_note_metadata.action).to eq(action)
expect(subject.system_note_metadata.commit_count).to eq(commit_count)
end
end
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