Commit 4f8ca3f7 authored by Luke Duncalfe's avatar Luke Duncalfe Committed by Imre Farkas

Refactor Issuable::Clone::BaseService to services

Previously Issuable::Clone::BaseService copied an issuable notes,
award emoji, and award emoji on the issuable notes to another issue
through Issuable::Clone::ContentRewriter.

In order to implement
https://gitlab.com/gitlab-org/gitlab/-/issues/13426 in future, we need
to be able to copy design notes and their award emoji independently of
Issuable::Clone::ContentRewriter which was highly-coupled to issuables.

Issuable::Clone::BaseService now calls new services:

- Notes::CopyService
- AwardEmoji::CopyService
- ContentRewriterService

Which contain re-usable logic to perform those actions.
parent 97b87392
# frozen_string_literal: true
# This service copies AwardEmoji from one Awardable to another.
#
# It expects the calling code to have performed the necessary authorization
# checks in order to allow the copy to happen.
module AwardEmojis
class CopyService
def initialize(from_awardable, to_awardable)
raise ArgumentError, 'Awardables must be different' if from_awardable == to_awardable
@from_awardable = from_awardable
@to_awardable = to_awardable
end
def execute
from_awardable.award_emoji.find_each do |award|
new_award = award.dup
new_award.awardable = to_awardable
new_award.save!
end
ServiceResponse.success
end
private
attr_accessor :from_awardable, :to_awardable
end
end
......@@ -24,12 +24,34 @@ module Issuable
private
def copy_award_emoji
AwardEmojis::CopyService.new(original_entity, new_entity).execute
end
def copy_notes
Notes::CopyService.new(current_user, original_entity, new_entity).execute
end
def update_new_entity
rewriters = [ContentRewriter, AttributesRewriter]
update_new_entity_description
update_new_entity_attributes
copy_award_emoji
copy_notes
end
rewriters.each do |rewriter|
rewriter.new(current_user, original_entity, new_entity).execute
end
def update_new_entity_description
rewritten_description = MarkdownContentRewriterService.new(
current_user,
original_entity.description,
original_entity.project,
new_entity.project
).execute
new_entity.update!(description: rewritten_description)
end
def update_new_entity_attributes
AttributesRewriter.new(current_user, original_entity, new_entity).execute
end
def update_old_entity
......
# frozen_string_literal: true
module Issuable
module Clone
class ContentRewriter < ::Issuable::Clone::BaseService
def initialize(current_user, original_entity, new_entity)
@current_user = current_user
@original_entity = original_entity
@new_entity = new_entity
@project = original_entity.project
end
def execute
rewrite_description
rewrite_award_emoji(original_entity, new_entity)
rewrite_notes
end
private
def rewrite_description
new_entity.update(description: rewrite_content(original_entity.description))
end
def rewrite_notes
new_discussion_ids = {}
original_entity.notes_with_associations.find_each do |note|
new_note = note.dup
new_discussion_ids[note.discussion_id] ||= Discussion.discussion_id(new_note)
new_params = {
project: new_entity.project,
noteable: new_entity,
discussion_id: new_discussion_ids[note.discussion_id],
note: rewrite_content(new_note.note),
note_html: nil,
created_at: note.created_at,
updated_at: note.updated_at
}
if note.system_note_metadata
new_params[:system_note_metadata] = note.system_note_metadata.dup
# TODO: Implement copying of description versions when an issue is moved
# https://gitlab.com/gitlab-org/gitlab/issues/32300
new_params[:system_note_metadata].description_version = nil
end
new_note.update(new_params)
rewrite_award_emoji(note, new_note)
end
end
def rewrite_content(content)
return unless content
rewriters = [Gitlab::Gfm::ReferenceRewriter, Gitlab::Gfm::UploadsRewriter]
rewriters.inject(content) do |text, klass|
rewriter = klass.new(text, old_project, current_user)
rewriter.rewrite(new_parent)
end
end
def rewrite_award_emoji(old_awardable, new_awardable)
old_awardable.award_emoji.each do |award|
new_award = award.dup
new_award.awardable = new_awardable
new_award.save
end
end
end
end
end
# frozen_string_literal: true
# This service passes Markdown content through our GFM rewriter classes
# which rewrite references to GitLab objects and uploads within the content
# based on their visibility by the `target_parent`.
class MarkdownContentRewriterService
REWRITERS = [Gitlab::Gfm::ReferenceRewriter, Gitlab::Gfm::UploadsRewriter].freeze
def initialize(current_user, content, source_parent, target_parent)
@current_user = current_user
@content = content.presence
@source_parent = source_parent
@target_parent = target_parent
end
def execute
return unless content
REWRITERS.inject(content) do |text, klass|
rewriter = klass.new(text, source_parent, current_user)
rewriter.rewrite(target_parent)
end
end
private
attr_reader :current_user, :content, :source_parent, :target_parent
end
# frozen_string_literal: true
# This service copies Notes from one Noteable to another.
#
# It expects the calling code to have performed the necessary authorization
# checks in order to allow the copy to happen.
module Notes
class CopyService
def initialize(current_user, from_noteable, to_noteable)
raise ArgumentError, 'Noteables must be different' if from_noteable == to_noteable
@current_user = current_user
@from_noteable = from_noteable
@to_noteable = to_noteable
@from_project = from_noteable.project
@to_project = to_noteable.project
@new_discussion_ids = {}
end
def execute
from_noteable.notes_with_associations.find_each do |note|
copy_note(note)
end
ServiceResponse.success
end
private
attr_reader :from_noteable, :to_noteable, :from_project, :to_project,
:current_user, :new_discussion_ids
def copy_note(note)
new_note = note.dup
new_params = params_from_note(note, new_note)
new_note.update!(new_params)
copy_award_emoji(note, new_note)
end
def params_from_note(note, new_note)
new_discussion_ids[note.discussion_id] ||= Discussion.discussion_id(new_note)
rewritten_note = MarkdownContentRewriterService.new(current_user, note.note, from_project, to_project).execute
new_params = {
project: to_noteable.project,
noteable: to_noteable,
discussion_id: new_discussion_ids[note.discussion_id],
note: rewritten_note,
note_html: nil,
created_at: note.created_at,
updated_at: note.updated_at
}
if note.system_note_metadata
new_params[:system_note_metadata] = note.system_note_metadata.dup
# TODO: Implement copying of description versions when an issue is moved
# https://gitlab.com/gitlab-org/gitlab/issues/32300
new_params[:system_note_metadata].description_version = nil
end
new_params
end
def copy_award_emoji(from_note, to_note)
AwardEmojis::CopyService.new(from_note, to_note).execute
end
end
end
......@@ -110,6 +110,20 @@ RSpec.describe Gitlab::Gfm::ReferenceRewriter do
end
end
context 'when description contains a local reference' do
let(:local_issue) { create(:issue, project: old_project) }
let(:text) { "See ##{local_issue.iid}" }
it { is_expected.to eq("See #{old_project.path}##{local_issue.iid}") }
end
context 'when description contains a cross reference' do
let(:merge_request) { create(:merge_request) }
let(:text) { "See #{merge_request.project.full_path}!#{merge_request.iid}" }
it { is_expected.to eq(text) }
end
context 'with a commit' do
let(:old_project) { create(:project, :repository, name: 'old-project', group: group) }
let(:commit) { old_project.commit }
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe AwardEmojis::CopyService do
let_it_be(:from_awardable) do
create(:issue, award_emoji: [
build(:award_emoji, name: 'thumbsup'),
build(:award_emoji, name: 'thumbsdown')
])
end
describe '#initialize' do
it 'validates that we cannot copy AwardEmoji to the same Awardable' do
expect { described_class.new(from_awardable, from_awardable) }.to raise_error(ArgumentError)
end
end
describe '#execute' do
let(:to_awardable) { create(:issue) }
subject(:execute_service) { described_class.new(from_awardable, to_awardable).execute }
it 'copies AwardEmojis', :aggregate_failures do
expect { execute_service }.to change { AwardEmoji.count }.by(2)
expect(to_awardable.award_emoji.map(&:name)).to match_array(%w(thumbsup thumbsdown))
end
it 'returns success', :aggregate_failures do
expect(execute_service).to be_kind_of(ServiceResponse)
expect(execute_service).to be_success
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Issuable::Clone::ContentRewriter do
let(:user) { create(:user) }
let(:group) { create(:group) }
let(:project1) { create(:project, :public, group: group) }
let(:project2) { create(:project, :public, group: group) }
let(:other_issue) { create(:issue, project: project1) }
let(:merge_request) { create(:merge_request) }
subject { described_class.new(user, original_issue, new_issue)}
let(:description) { 'Simple text' }
let(:original_issue) { create(:issue, description: description, project: project1) }
let(:new_issue) { create(:issue, project: project2) }
context 'rewriting award emojis' do
it 'copies the award emojis' do
create(:award_emoji, awardable: original_issue, name: 'thumbsup')
create(:award_emoji, awardable: original_issue, name: 'thumbsdown')
expect { subject.execute }.to change { AwardEmoji.count }.by(2)
expect(new_issue.award_emoji.map(&:name)).to match_array(%w(thumbsup thumbsdown))
end
end
context 'rewriting description' do
before do
subject.execute
end
context 'when description is a simple text' do
it 'does not rewrite the description' do
expect(new_issue.reload.description).to eq(original_issue.description)
end
end
context 'when description contains a local reference' do
let(:description) { "See ##{other_issue.iid}" }
it 'rewrites the local reference correctly' do
expected_description = "See #{project1.path}##{other_issue.iid}"
expect(new_issue.reload.description).to eq(expected_description)
end
end
context 'when description contains a cross reference' do
let(:description) { "See #{merge_request.project.full_path}!#{merge_request.iid}" }
it 'rewrites the cross reference correctly' do
expected_description = "See #{merge_request.project.full_path}!#{merge_request.iid}"
expect(new_issue.reload.description).to eq(expected_description)
end
end
context 'when description contains a user reference' do
let(:description) { "FYU #{user.to_reference}" }
it 'works with a user reference' do
expect(new_issue.reload.description).to eq("FYU #{user.to_reference}")
end
end
context 'when description contains uploads' do
let(:uploader) { build(:file_uploader, project: project1) }
let(:description) { "Text and #{uploader.markdown_link}" }
it 'rewrites uploads in the description' do
upload = Upload.last
expect(new_issue.description).not_to eq(description)
expect(new_issue.description).to match(/Text and #{FileUploader::MARKDOWN_PATTERN}/)
expect(upload.secret).not_to eq(uploader.secret)
expect(new_issue.description).to include(upload.secret)
expect(new_issue.description).to include(upload.path)
end
end
end
context 'rewriting notes' do
context 'simple notes' do
let!(:notes) do
[
create(:note, noteable: original_issue, project: project1,
created_at: 2.weeks.ago, updated_at: 1.week.ago),
create(:note, noteable: original_issue, project: project1),
create(:note, system: true, noteable: original_issue, project: project1)
]
end
let!(:system_note_metadata) { create(:system_note_metadata, note: notes.last) }
let!(:award_emoji) { create(:award_emoji, awardable: notes.first, name: 'thumbsup')}
before do
subject.execute
end
it 'rewrites existing notes in valid order' do
expect(new_issue.notes.order('id ASC').pluck(:note).first(3)).to eq(notes.map(&:note))
end
it 'copies all the issue notes' do
expect(new_issue.notes.count).to eq(3)
end
it 'does not change the note attributes' do
subject.execute
new_note = new_issue.notes.first
expect(new_note.note).to eq(notes.first.note)
expect(new_note.author).to eq(notes.first.author)
end
it 'copies the award emojis' do
subject.execute
new_note = new_issue.notes.first
new_note.award_emoji.first.name = 'thumbsup'
end
it 'copies system_note_metadata for system note' do
new_note = new_issue.notes.last
expect(new_note.system_note_metadata.action).to eq(system_note_metadata.action)
expect(new_note.system_note_metadata.id).not_to eq(system_note_metadata.id)
end
end
context 'notes with reference' do
let(:text) do
"See ##{other_issue.iid} and #{merge_request.project.full_path}!#{merge_request.iid}"
end
let!(:note) { create(:note, noteable: original_issue, note: text, project: project1) }
it 'rewrites the references correctly' do
subject.execute
new_note = new_issue.notes.first
expected_text = "See #{other_issue.project.path}##{other_issue.iid} and #{merge_request.project.full_path}!#{merge_request.iid}"
expect(new_note.note).to eq(expected_text)
expect(new_note.author).to eq(note.author)
end
end
context 'notes with upload' do
let(:uploader) { build(:file_uploader, project: project1) }
let(:text) { "Simple text with image: #{uploader.markdown_link} "}
let!(:note) { create(:note, noteable: original_issue, note: text, project: project1) }
it 'rewrites note content correctly' do
subject.execute
new_note = new_issue.notes.first
expect(note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/)
expect(new_note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/)
expect(note.note).not_to eq(new_note.note)
expect(note.note_html).not_to eq(new_note.note_html)
end
end
context "discussion notes" do
let(:note) { create(:note, noteable: original_issue, note: "sample note", project: project1) }
let!(:discussion) { create(:discussion_note_on_issue, in_reply_to: note, note: "reply to sample note") }
it 'rewrites discussion correctly' do
subject.execute
expect(new_issue.notes.count).to eq(original_issue.notes.count)
expect(new_issue.notes.where(discussion_id: discussion.discussion_id).count).to eq(0)
expect(original_issue.notes.where(discussion_id: discussion.discussion_id).count).to eq(1)
end
end
end
end
......@@ -3,15 +3,15 @@
require 'spec_helper'
RSpec.describe Issues::MoveService do
let(:user) { create(:user) }
let(:author) { create(:user) }
let(:title) { 'Some issue' }
let(:description) { "Some issue description with mention to #{user.to_reference}" }
let(:group) { create(:group, :private) }
let(:sub_group_1) { create(:group, :private, parent: group) }
let(:sub_group_2) { create(:group, :private, parent: group) }
let(:old_project) { create(:project, namespace: sub_group_1) }
let(:new_project) { create(:project, namespace: sub_group_2) }
let_it_be(:user) { create(:user) }
let_it_be(:author) { create(:user) }
let_it_be(:title) { 'Some issue' }
let_it_be(:description) { "Some issue description with mention to #{user.to_reference}" }
let_it_be(:group) { create(:group, :private) }
let_it_be(:sub_group_1) { create(:group, :private, parent: group) }
let_it_be(:sub_group_2) { create(:group, :private, parent: group) }
let_it_be(:old_project) { create(:project, namespace: sub_group_1) }
let_it_be(:new_project) { create(:project, namespace: sub_group_2) }
let(:old_issue) do
create(:issue, title: title, description: description, project: old_project, author: author)
......@@ -30,15 +30,10 @@ RSpec.describe Issues::MoveService do
describe '#execute' do
shared_context 'issue move executed' do
let!(:award_emoji) { create(:award_emoji, awardable: old_issue) }
let!(:new_issue) { move_service.execute(old_issue, new_project) }
end
context 'issue movable' do
let!(:note_with_mention) { create(:note, noteable: old_issue, author: author, project: old_project, note: "note with mention #{user.to_reference}") }
let!(:note_with_no_mention) { create(:note, noteable: old_issue, author: author, project: old_project, note: "note without mention") }
include_context 'user can move issue'
context 'generic issue' do
......@@ -48,11 +43,11 @@ RSpec.describe Issues::MoveService do
expect(new_issue.project).to eq new_project
end
it 'rewrites issue title' do
it 'copies issue title' do
expect(new_issue.title).to eq title
end
it 'rewrites issue description' do
it 'copies issue description' do
expect(new_issue.description).to eq description
end
......@@ -93,23 +88,21 @@ RSpec.describe Issues::MoveService do
it 'preserves create time' do
expect(old_issue.created_at).to eq new_issue.created_at
end
end
it 'moves the award emoji' do
expect(old_issue.award_emoji.first.name).to eq new_issue.reload.award_emoji.first.name
end
context 'issue with award emoji' do
let!(:award_emoji) { create(:award_emoji, awardable: old_issue) }
context 'when issue has notes with mentions' do
it 'saves user mentions with actual mentions for new issue' do
expect(new_issue.user_mentions.find_by(note_id: nil).mentioned_users_ids).to match_array([user.id])
expect(new_issue.user_mentions.where.not(note_id: nil).first.mentioned_users_ids).to match_array([user.id])
expect(new_issue.user_mentions.where.not(note_id: nil).count).to eq 1
expect(new_issue.user_mentions.count).to eq 2
end
it 'copies the award emoji' do
old_issue.reload
new_issue = move_service.execute(old_issue, new_project)
expect(old_issue.award_emoji.first.name).to eq new_issue.reload.award_emoji.first.name
end
end
context 'issue with assignee' do
let(:assignee) { create(:user) }
let_it_be(:assignee) { create(:user) }
before do
old_issue.assignees = [assignee]
......@@ -154,6 +147,25 @@ RSpec.describe Issues::MoveService do
.not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError
end
end
# These tests verify that notes are copied. More thorough tests are in
# the unit test for Notes::CopyService.
context 'issue with notes' do
let!(:notes) do
[
create(:note, noteable: old_issue, project: old_project, created_at: 2.weeks.ago, updated_at: 1.week.ago),
create(:note, noteable: old_issue, project: old_project)
]
end
let(:copied_notes) { new_issue.notes.limit(notes.size) } # Remove the system note added by the copy itself
include_context 'issue move executed'
it 'copies existing notes in order' do
expect(copied_notes.order('id ASC').pluck(:note)).to eq(notes.map(&:note))
end
end
end
describe 'move permissions' do
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe MarkdownContentRewriterService do
describe '#execute' do
let_it_be(:user) { create(:user) }
let_it_be(:source_parent) { create(:project, :public) }
let_it_be(:target_parent) { create(:project, :public) }
let(:content) { 'My content' }
subject { described_class.new(user, content, source_parent, target_parent).execute }
it 'calls the rewriter classes successfully', :aggregate_failures do
[Gitlab::Gfm::ReferenceRewriter, Gitlab::Gfm::UploadsRewriter].each do |rewriter_class|
service = double
expect(service).to receive(:rewrite).with(target_parent)
expect(rewriter_class).to receive(:new).and_return(service)
end
subject
end
# Perform simple integration-style tests for each rewriter class.
# to prove they run correctly.
context 'when content contains a reference' do
let_it_be(:issue) { create(:issue, project: source_parent) }
let(:content) { "See ##{issue.iid}" }
it 'rewrites content' do
expect(subject).to eq("See #{source_parent.full_path}##{issue.iid}")
end
end
context 'when content contains an upload' do
let(:image_uploader) { build(:file_uploader, project: source_parent) }
let(:content) { "Text and #{image_uploader.markdown_link}" }
it 'rewrites content' do
new_content = subject
expect(new_content).not_to eq(content)
expect(new_content.length).to eq(content.length)
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Notes::CopyService do
describe '#initialize' do
let_it_be(:noteable) { create(:issue) }
it 'validates that we cannot copy notes to the same Noteable' do
expect { described_class.new(noteable, noteable) }.to raise_error(ArgumentError)
end
end
describe '#execute' do
let_it_be(:user) { create(:user) }
let_it_be(:group) { create(:group) }
let_it_be(:from_project) { create(:project, :public, group: group) }
let_it_be(:to_project) { create(:project, :public, group: group) }
let(:from_noteable) { create(:issue, project: from_project) }
let(:to_noteable) { create(:issue, project: to_project) }
subject(:execute_service) { described_class.new(user, from_noteable, to_noteable).execute }
context 'rewriting the note body' do
context 'simple notes' do
let!(:notes) do
[
create(:note, noteable: from_noteable, project: from_noteable.project,
created_at: 2.weeks.ago, updated_at: 1.week.ago),
create(:note, noteable: from_noteable, project: from_noteable.project),
create(:note, system: true, noteable: from_noteable, project: from_noteable.project)
]
end
it 'rewrites existing notes in valid order' do
execute_service
expect(to_noteable.notes.order('id ASC').pluck(:note).first(3)).to eq(notes.map(&:note))
end
it 'copies all the issue notes' do
execute_service
expect(to_noteable.notes.count).to eq(3)
end
it 'does not change the note attributes' do
execute_service
new_note = to_noteable.notes.first
expect(new_note).to have_attributes(
note: notes.first.note,
author: notes.first.author
)
end
it 'copies the award emojis' do
create(:award_emoji, awardable: notes.first, name: 'thumbsup')
execute_service
new_award_emoji = to_noteable.notes.first.award_emoji.first
expect(new_award_emoji.name).to eq('thumbsup')
end
it 'copies system_note_metadata for system note' do
system_note_metadata = create(:system_note_metadata, note: notes.last)
execute_service
new_note = to_noteable.notes.last
aggregate_failures do
expect(new_note.system_note_metadata.action).to eq(system_note_metadata.action)
expect(new_note.system_note_metadata.id).not_to eq(system_note_metadata.id)
end
end
it 'returns success' do
aggregate_failures do
expect(execute_service).to be_kind_of(ServiceResponse)
expect(execute_service).to be_success
end
end
end
context 'notes with mentions' do
let!(:note_with_mention) { create(:note, noteable: from_noteable, author: from_noteable.author, project: from_noteable.project, note: "note with mention #{user.to_reference}") }
let!(:note_with_no_mention) { create(:note, noteable: from_noteable, author: from_noteable.author, project: from_noteable.project, note: "note without mention") }
it 'saves user mentions with actual mentions for new issue' do
execute_service
aggregate_failures do
expect(to_noteable.user_mentions.first.mentioned_users_ids).to match_array([user.id])
expect(to_noteable.user_mentions.count).to eq(1)
end
end
end
context 'notes with reference' do
let(:other_issue) { create(:issue, project: from_noteable.project) }
let(:merge_request) { create(:merge_request) }
let(:text) { "See ##{other_issue.iid} and #{merge_request.project.full_path}!#{merge_request.iid}" }
let!(:note) { create(:note, noteable: from_noteable, note: text, project: from_noteable.project) }
it 'rewrites the references correctly' do
execute_service
new_note = to_noteable.notes.first
expected_text = "See #{other_issue.project.path}##{other_issue.iid} and #{merge_request.project.full_path}!#{merge_request.iid}"
aggregate_failures do
expect(new_note.note).to eq(expected_text)
expect(new_note.author).to eq(note.author)
end
end
end
context 'notes with upload' do
let(:uploader) { build(:file_uploader, project: from_noteable.project) }
let(:text) { "Simple text with image: #{uploader.markdown_link} "}
let!(:note) { create(:note, noteable: from_noteable, note: text, project: from_noteable.project) }
it 'rewrites note content correctly' do
execute_service
new_note = to_noteable.notes.first
aggregate_failures do
expect(note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/)
expect(new_note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/)
expect(note.note).not_to eq(new_note.note)
expect(note.note_html).not_to eq(new_note.note_html)
end
end
end
context 'discussion notes' do
let(:note) { create(:note, noteable: from_noteable, note: 'sample note', project: from_noteable.project) }
let!(:discussion) { create(:discussion_note_on_issue, in_reply_to: note, note: 'reply to sample note') }
it 'rewrites discussion correctly' do
execute_service
aggregate_failures do
expect(to_noteable.notes.count).to eq(from_noteable.notes.count)
expect(to_noteable.notes.where(discussion_id: discussion.discussion_id).count).to eq(0)
expect(from_noteable.notes.where(discussion_id: discussion.discussion_id).count).to eq(1)
end
end
end
end
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