Skip to content

G
gitlab-ce
Commit be0877c6 authored by Patrick Derichs's avatar Patrick Derichs
Browse files
Options

Extract Commit System Notes Service 

One part of SystemNoteService refactoring.

Move specs to SystemNotes::CommitService

Add BaseService.

Preparation for the follow-up MRs
regarding SystemNoteService refactoring.
Specs are using doubles

Also adding specs for BaseService and
changed scope of some methods to protected.

Brought back accidentally removed comment
to document return value.

Use expect_next_instance_of instead of 
expect_any_instance_of

Apply suggestion to 
spec/services/system_note_service_spec.rb

Apply suggestion to 
spec/services/system_note_service_spec.rb
parent 95017820
Hide whitespace changes
Inline Side-by-side
Showing with 328 additions and 208 deletions
app/services/system_note_service.rb
View file @ be0877c6
......@@ -16,20 +16,9 @@ module SystemNoteService
# 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(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)
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))
::SystemNotes::CommitService.new(noteable, project, author).add_commits(new_commits, existing_commits, oldrev)
end
# Called when a commit was tagged
......@@ -41,10 +30,7 @@ module SystemNoteService
#
# Returns the created Note object
def tag_commit(noteable, project, author, 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'))
::SystemNotes::CommitService.new(noteable, project, author).tag_commit(tag_name)
end
# Called when the assignee of a Noteable is changed or removed
......@@ -497,17 +483,6 @@ module SystemNoteService
notes_for_mentioner(mentioner, noteable, notes).exists?
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
#
# noteable - Noteable object.
......@@ -637,71 +612,10 @@ module SystemNoteService
"#{cross_reference_note_prefix}#{gfm_reference}"
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
@url_helpers ||= Gitlab::Routing.url_helpers
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)
ActionController::Base.helpers.content_tag(*args)
end
......
app/services/system_notes/base_service.rb 0 → 100644
View file @ be0877c6
# frozen_string_literal: true
module SystemNotes
class BaseService
attr_reader :noteable, :project, :author
def initialize(noteable, project, author)
@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 @ be0877c6
# 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 @ be0877c6
......@@ -14,127 +14,29 @@ describe SystemNoteService do
let(:noteable) { create(:issue, project: project) }
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
subject { described_class.add_commits(noteable, project, author, 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
let(:new_commits) { double }
let(:old_commits) { double }
let(:oldrev) { double }
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
it 'calls CommitService' do
expect_next_instance_of(::SystemNotes::CommitService) do |service|
expect(service).to receive(:add_commits).with(new_commits, old_commits, oldrev)
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
described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev)
end
end
describe '.tag_commit' do
let(:noteable) do
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
let(:tag_name) { double }
it 'sets the note text' do
link = "/#{project.full_path}/-/tags/#{tag_name}"
it 'calls CommitService' do
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
......@@ -804,15 +706,6 @@ describe SystemNoteService do
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
include JiraServiceHelper
......
spec/services/system_notes/base_service_spec.rb 0 → 100644
View file @ be0877c6
# 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, project, author) }
describe '#noteable' do
subject { base_service.noteable }
it { is_expected.to eq(noteable) }
end
describe '#project' do
subject { base_service.project }
it { is_expected.to eq(project) }
end
describe '#author' do
subject { base_service.author }
it { is_expected.to eq(author) }
end
end
spec/services/system_notes/commit_service_spec.rb 0 → 100644
View file @ be0877c6
# 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, project, 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(nil, nil, nil).new_commit_summary([commit])).to all(match(/- #{escaped}/))
end
end
end
spec/support/shared_examples/common_system_notes_examples.rb
View file @ be0877c6
......@@ -27,3 +27,28 @@ shared_examples 'WIP notes creation' do |wip_action|
expect(Note.second.note).to match('changed title')
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