Commit 0a8a30d3 authored by Luke Duncalfe's avatar Luke Duncalfe Committed by Stan Hu

Copy designs to new issue when issue is moved

Adds a set of services and workers to allow an asynchronous copy of
designs from one issue to another.

It happens asynchronously from the in-request issue move as the copy
can take around 15s for 50 designs.

The copy involves copying designs, versions and discussions on designs.

https://gitlab.com/gitlab-org/gitlab/-/issues/13426
parent 57a9b293
......@@ -167,6 +167,10 @@ module DesignManagement
end
end
def self.build_full_path(issue, design)
File.join(DesignManagement.designs_directory, "issue-#{issue.iid}", design.filename)
end
def to_ability_name
'design'
end
......@@ -180,7 +184,7 @@ module DesignManagement
end
def full_path
@full_path ||= File.join(DesignManagement.designs_directory, "issue-#{issue.iid}", filename)
@full_path ||= self.class.build_full_path(issue, self)
end
def diff_refs
......@@ -224,6 +228,10 @@ module DesignManagement
!interloper.exists?
end
def notes_with_associations
notes.includes(:author)
end
private
def head_version
......
......@@ -5,6 +5,7 @@ module DesignManagement
attr_reader :issue
delegate :designs, :project, to: :issue
delegate :empty?, to: :designs
state_machine :copy_state, initial: :ready, namespace: :copy do
after_transition any => any, do: :update_stored_copy_state!
......
# frozen_string_literal: true
module DesignManagement
module CopyDesignCollection
end
end
# frozen_string_literal: true
# Service for setting the initial copy_state on the target DesignCollection
# and queuing a CopyDesignCollectionWorker.
module DesignManagement
module CopyDesignCollection
class QueueService
def initialize(current_user, issue, target_issue)
@current_user = current_user
@issue = issue
@target_issue = target_issue
@target_design_collection = target_issue.design_collection
end
def execute
return error('User cannot copy designs to issue') unless user_can_copy?
return error('Target design collection copy state must be `ready`') unless target_design_collection.can_start_copy?
target_design_collection.start_copy!
DesignManagement::CopyDesignCollectionWorker.perform_async(current_user.id, issue.id, target_issue.id)
ServiceResponse.success
end
private
delegate :design_collection, to: :issue
attr_reader :current_user, :issue, :target_design_collection, :target_issue
def error(message)
ServiceResponse.error(message: message)
end
def user_can_copy?
current_user.can?(:read_design, issue) &&
current_user.can?(:admin_issue, target_issue)
end
end
end
end
......@@ -19,6 +19,7 @@ module DesignManagement
def collection
issue.design_collection
end
alias_method :design_collection, :collection
def repository
collection.repository
......
......@@ -4,14 +4,15 @@ module DesignManagement
module RunsDesignActions
NoActions = Class.new(StandardError)
# this concern requires the following methods to be implemented:
# This concern requires the following methods to be implemented:
# current_user, target_branch, repository, commit_message
#
# Before calling `run_actions`, you should ensure the repository exists, by
# calling `repository.create_if_not_exists`.
#
# @raise [NoActions] if actions are empty
def run_actions(actions)
# @return [DesignManagement::Version]
def run_actions(actions, skip_system_notes: false)
raise NoActions if actions.empty?
sha = repository.multi_action(current_user,
......@@ -21,14 +22,14 @@ module DesignManagement
::DesignManagement::Version
.create_for_designs(actions, sha, current_user)
.tap { |version| post_process(version) }
.tap { |version| post_process(version, skip_system_notes) }
end
private
def post_process(version)
def post_process(version, skip_system_notes)
version.run_after_commit_or_now do
::DesignManagement::NewVersionWorker.perform_async(id)
::DesignManagement::NewVersionWorker.perform_async(id, skip_system_notes)
end
end
end
......
......@@ -17,11 +17,14 @@ module DesignManagement
return error("Not allowed!") unless can_create_designs?
return error("Only #{MAX_FILES} files are allowed simultaneously") if files.size > MAX_FILES
return error("Duplicate filenames are not allowed!") if files.map(&:original_filename).uniq.length != files.length
return error("Design copy is in progress") if design_collection.copy_in_progress?
uploaded_designs, version = upload_designs!
skipped_designs = designs - uploaded_designs
create_events
design_collection.reset_copy!
success({ designs: uploaded_designs, version: version, skipped_designs: skipped_designs })
rescue ::ActiveRecord::RecordInvalid => e
error(e.message)
......@@ -35,7 +38,10 @@ module DesignManagement
::DesignManagement::Version.with_lock(project.id, repository) do
actions = build_actions
[actions.map(&:design), actions.presence && run_actions(actions)]
[
actions.map(&:design),
actions.presence && run_actions(actions)
]
end
end
......
......@@ -23,11 +23,15 @@ module Issues
# to receive service desk emails on the new moved issue.
update_service_desk_sent_notifications
queue_copy_designs
new_entity
end
private
attr_reader :target_project
def update_service_desk_sent_notifications
return unless original_entity.from_service_desk?
......@@ -46,7 +50,7 @@ module Issues
new_params = {
id: nil,
iid: nil,
project: @target_project,
project: target_project,
author: original_entity.author,
assignee_ids: original_entity.assignee_ids
}
......@@ -58,6 +62,23 @@ module Issues
CreateService.new(@target_project, @current_user, new_params).execute(skip_system_notes: true)
end
def queue_copy_designs
return unless copy_designs_enabled? && original_entity.designs.present?
response = DesignManagement::CopyDesignCollection::QueueService.new(
current_user,
original_entity,
new_entity
).execute
log_error(response.message) if response.error?
end
def copy_designs_enabled?
Feature.enabled?(:design_management_copy_designs, old_project) &&
Feature.enabled?(:design_management_copy_designs, target_project)
end
def mark_as_moved
original_entity.update(moved_to: new_entity)
end
......@@ -75,7 +96,7 @@ module Issues
end
def add_note_from
SystemNoteService.noteable_moved(new_entity, @target_project,
SystemNoteService.noteable_moved(new_entity, target_project,
original_entity, current_user,
direction: :from)
end
......
......@@ -1332,6 +1332,14 @@
:weight: 1
:idempotent:
:tags: []
- :name: design_management_copy_design_collection
:feature_category: :design_management
:has_external_dependencies:
:urgency: :low
:resource_boundary: :unknown
:weight: 1
:idempotent: true
:tags: []
- :name: design_management_new_version
:feature_category: :design_management
:has_external_dependencies:
......
# frozen_string_literal: true
module DesignManagement
class CopyDesignCollectionWorker
include ApplicationWorker
feature_category :design_management
idempotent!
urgency :low
def perform(user_id, issue_id, target_issue_id)
user = User.find(user_id)
issue = Issue.find(issue_id)
target_issue = Issue.find(target_issue_id)
response = DesignManagement::CopyDesignCollection::CopyService.new(
target_issue.project,
user,
issue: issue,
target_issue: target_issue
).execute
Gitlab::AppLogger.warn(response.message) if response.error?
end
end
end
......@@ -9,10 +9,10 @@ module DesignManagement
# `GenerateImageVersionsService` resizing designs
worker_resource_boundary :memory
def perform(version_id)
def perform(version_id, skip_system_notes = false)
version = DesignManagement::Version.find(version_id)
add_system_note(version)
add_system_note(version) unless skip_system_notes
generate_image_versions(version)
rescue ActiveRecord::RecordNotFound => e
Sidekiq.logger.warn(e)
......
---
title: Copy designs to new issue when issue is moved
merge_request: 41714
author:
type: added
---
name: design_management_copy_designs
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/41714
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/247062
group: group::knowledge
type: development
default_enabled: false
\ No newline at end of file
......@@ -76,6 +76,8 @@
- 1
- - deployment
- 3
- - design_management_copy_design_collection
- 1
- - design_management_new_version
- 1
- - detect_repository_languages
......
# This file exists to lock the attributes of Design Management models
# that get copied in `DesignManagement::CopyDesignCollection::CopyService`
# to specific schemas.
#
# This allows us to perform sanity checks and alert when there are changes
# to the schema by running expectations against the lists in this file
# and the actual schema of the models in `copy_designs_service_spec.rb`.
#
# If you are here because you received a failed test in
# `copy_designs_service_spec.rb`, you need to decide how to handle the
# changes and whether the new attribute(s) should be included in the copy
# or ignored.
# COPY.
# Add attributes that should be copied to the `{model}_attributes` lists:
design_attributes:
- filename
- relative_position
version_attributes:
- author_id
- created_at
action_attributes: # (None)
# IGNORE.
# Add attributes that should not be copied to the `ignore_{model}_attributes` lists:
ignore_design_attributes:
- id
- issue_id
- project_id
ignore_version_attributes:
- id
- issue_id
- sha
ignore_action_attributes:
- id
- design_id
- event
- image_v432x230
- version_id
......@@ -101,6 +101,18 @@ RSpec.describe DesignManagement::DesignCollection do
end
end
describe "#empty?" do
it "is true when the design collection has no designs" do
expect(collection).to be_empty
end
it "is false when the design collection has designs" do
create(:design, issue: issue)
expect(collection).not_to be_empty
end
end
describe "#versions" do
it "includes versions for all designs" do
version_1 = create(:design_version)
......
......@@ -206,6 +206,15 @@ RSpec.describe DesignManagement::Design do
end
end
describe ".build_full_path" do
it "builds the full path for a design" do
design = build(:design, issue: issue, filename: "hello.jpg")
expected_path = "#{DesignManagement.designs_directory}/issue-#{design.issue.iid}/hello.jpg"
expect(described_class.build_full_path(issue, design)).to eq(expected_path)
end
end
describe '#visible_in?' do
let_it_be(:issue) { create(:issue, project: issue.project) }
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitlab_redis_shared_state do
include DesignManagementTestHelpers
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project) }
let_it_be(:issue, refind: true) { create(:issue, project: project) }
let(:target_issue) { create(:issue) }
subject { described_class.new(project, user, issue: issue, target_issue: target_issue).execute }
before do
enable_design_management
end
shared_examples 'service error' do |message:|
it 'returns an error response', :aggregate_failures do
expect(subject).to be_kind_of(ServiceResponse)
expect(subject).to be_error
expect(subject.message).to eq(message)
end
end
shared_examples 'service success' do
it 'returns a success response', :aggregate_failures do
expect(subject).to be_kind_of(ServiceResponse)
expect(subject).to be_success
end
end
include_examples 'service error', message: 'User cannot copy design collection to issue'
context 'when user has permission to read the design collection' do
before_all do
project.add_reporter(user)
end
include_examples 'service error', message: 'User cannot copy design collection to issue'
context 'when the user also has permission to admin the target issue' do
let(:target_repository) { target_issue.project.design_repository }
before do
target_issue.project.add_reporter(user)
end
include_examples 'service error', message: 'Target design collection must first be queued'
context 'when the target design collection has been queued' do
before do
target_issue.design_collection.start_copy!
end
include_examples 'service error', message: 'Design collection has no designs'
context 'when design collection has designs' do
let_it_be(:designs) do
create_list(:design, 3, :with_lfs_file, :with_relative_position, issue: issue, project: project)
end
context 'when target issue already has designs' do
before do
create(:design, issue: target_issue, project: target_issue.project)
end
include_examples 'service error', message: 'Target design collection already has designs'
end
include_examples 'service success'
it 'creates a design repository for the target project' do
expect { subject }.to change { target_repository.exists? }.from(false).to(true)
end
context 'when the target project already has a design repository' do
before do
target_repository.create_if_not_exists
end
include_examples 'service success'
end
it 'copies the designs correctly', :aggregate_failures do
expect { subject }.to change { target_issue.designs.count }.by(3)
old_designs = issue.designs.ordered
new_designs = target_issue.designs.ordered
new_designs.zip(old_designs).each do |new_design, old_design|
expect(new_design).to have_attributes(
filename: old_design.filename,
relative_position: old_design.relative_position,
issue: target_issue,
project: target_issue.project
)
end
end
it 'copies the design versions correctly', :aggregate_failures do
expect { subject }.to change { target_issue.design_versions.count }.by(3)
old_versions = issue.design_versions.ordered
new_versions = target_issue.design_versions.ordered
new_versions.zip(old_versions).each do |new_version, old_version|
expect(new_version).to have_attributes(
created_at: old_version.created_at,
author_id: old_version.author_id
)
expect(new_version.designs.pluck(:filename)).to eq(old_version.designs.pluck(:filename))
expect(new_version.actions.pluck(:event)).to eq(old_version.actions.pluck(:event))
end
end
it 'copies the design actions correctly', :aggregate_failures do
expect { subject }.to change { DesignManagement::Action.count }.by(3)
old_actions = issue.design_versions.ordered.flat_map(&:actions)
new_actions = target_issue.design_versions.ordered.flat_map(&:actions)
new_actions.zip(old_actions).each do |new_action, old_action|
# This is a way to identify if the versions linked to the actions
# are correct is to compare design filenames, as the SHA changes.
new_design_filenames = new_action.version.designs.ordered.pluck(:filename)
old_design_filenames = old_action.version.designs.ordered.pluck(:filename)
expect(new_design_filenames).to eq(old_design_filenames)
expect(new_action.event).to eq(old_action.event)
expect(new_action.design.filename).to eq(old_action.design.filename)
end
end
it 'copies design notes correctly', :aggregate_failures, :sidekiq_inline do
note = create(:diff_note_on_design, noteable: designs.first, project: project)
expect { subject }.to change { Note.count }.by(1)
new_note = target_issue.designs.first.notes.first
expect(new_note).to have_attributes(
type: note.type,
author_id: note.author_id,
note: note.note,
position: note.position
)
end
it 'links the LfsObjects' do
expect { subject }.to change { target_issue.project.lfs_objects.count }.by(3)
end
it 'copies the Git repository data', :aggregate_failures do
subject
commit_shas = target_repository.commits('master', limit: 99).map(&:id)
expect(commit_shas).to include(*target_issue.design_versions.ordered.pluck(:sha))
end
it 'creates a master branch if none previously existed' do
expect { subject }.to change { target_repository.branch_names }.from([]).to(['master'])
end
it 'leaves the design collection in the correct copy state' do
subject
expect(target_issue.design_collection).to be_copy_ready
end
describe 'rollback' do
before do
# Ensure the very last step throws an error
expect_next_instance_of(described_class) do |service|
expect(service).to receive(:finalize!).and_raise
end
end
include_examples 'service error', message: 'Designs were unable to be copied successfully'
it 'rollsback all PostgreSQL data created', :aggregate_failures do
expect { subject }.not_to change {
[
DesignManagement::Design.count,
DesignManagement::Action.count,
DesignManagement::Version.count,
Note.count
]
}
collections = [
target_issue.design_collection,
target_issue.designs,
target_issue.design_versions
]
expect(collections).to all(be_empty)
end
it 'does not alter master branch', :aggregate_failures do
# Add some Git data to the target_repository, so we are testing
# that any original data remains
issue_2 = create(:issue, project: target_issue.project)
create(:design, :with_file, issue: issue_2, project: target_issue.project)
expect { subject }.not_to change {
expect(target_repository.commits('master', limit: 10).size).to eq(1)
}
end
it 'sets the design collection copy state' do
subject
expect(target_issue.design_collection).to be_copy_error
end
end
end
end
end
end
describe 'Alert if schema changes', :aggregate_failures do
let_it_be(:config_file) { Rails.root.join('lib/gitlab/design_management/copy_design_collection_model_attributes.yml') }
let_it_be(:config) { YAML.load_file(config_file).symbolize_keys }
%w(Design Action Version).each do |model|
specify do
attributes = config["#{model.downcase}_attributes".to_sym] || []
ignored_attributes = config["ignore_#{model.downcase}_attributes".to_sym]
expect(attributes + ignored_attributes).to contain_exactly(
*DesignManagement.const_get(model, false).column_names
), failure_message(model)
end
end
def failure_message(model)
<<-MSG
The schema of the `#{model}` model has changed.
`#{described_class.name}` refers to specific lists of attributes of `#{model}` to either
copy or ignore, so that we continue to copy designs correctly after schema changes.
Please update:
#{config_file}
to reflect the latest changes to `#{model}`. See that file for more information.
MSG
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe DesignManagement::CopyDesignCollection::QueueService, :clean_gitlab_redis_shared_state do
include DesignManagementTestHelpers
let_it_be(:user) { create(:user) }
let_it_be(:issue) { create(:issue) }
let_it_be(:target_issue, refind: true) { create(:issue) }
let_it_be(:design) { create(:design, issue: issue, project: issue.project) }
subject { described_class.new(user, issue, target_issue).execute }
before do
enable_design_management
end
it 'returns an error if user does not have permission' do
expect(subject).to be_kind_of(ServiceResponse)
expect(subject).to be_error
expect(subject.message).to eq('User cannot copy designs to issue')
end
context 'when user has permission' do
before_all do
issue.project.add_reporter(user)
target_issue.project.add_reporter(user)
end
it 'returns an error if design collection copy_state is not queuable' do
target_issue.design_collection.start_copy!
expect(subject).to be_kind_of(ServiceResponse)
expect(subject).to be_error
expect(subject.message).to eq('Target design collection copy state must be `ready`')
end
it 'sets the design collection copy state' do
expect { subject }.to change { target_issue.design_collection.copy_state }.from('ready').to('in_progress')
end
it 'queues a DesignManagement::CopyDesignCollectionWorker' do
expect { subject }.to change(DesignManagement::CopyDesignCollectionWorker.jobs, :size).by(1)
end
it 'returns success' do
expect(subject).to be_kind_of(ServiceResponse)
expect(subject).to be_success
end
end
end
......@@ -105,7 +105,7 @@ RSpec.describe DesignManagement::DeleteDesignsService do
end
it 'informs the new-version-worker' do
expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer)
expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer, false)
run_service
end
......
......@@ -32,7 +32,7 @@ RSpec.describe DesignManagement::SaveDesignsService do
end
allow(::DesignManagement::NewVersionWorker)
.to receive(:perform_async).with(Integer).and_return(nil)
.to receive(:perform_async).with(Integer, false).and_return(nil)
end
def run_service(files_to_upload = nil)
......@@ -128,6 +128,25 @@ RSpec.describe DesignManagement::SaveDesignsService do
expect { run_parallel(blocks) }.to change(DesignManagement::Version, :count).by(parellism)
end
context 'when the design collection is in the process of being copied', :clean_gitlab_redis_shared_state do
before do
issue.design_collection.start_copy!
end
it_behaves_like 'a service error'
end
context 'when the design collection has a copy error', :clean_gitlab_redis_shared_state do
before do
issue.design_collection.copy_state = 'error'
issue.design_collection.send(:set_stored_copy_state!)
end
it 'resets the copy state' do
expect { run_service }.to change { issue.design_collection.copy_state }.from('error').to('ready')
end
end
describe 'the response' do
it 'includes designs with the expected properties' do
updated_designs = response[:designs]
......@@ -220,7 +239,7 @@ RSpec.describe DesignManagement::SaveDesignsService do
counter = Gitlab::UsageDataCounters::DesignsCounter
expect(::DesignManagement::NewVersionWorker)
.to receive(:perform_async).once.with(Integer).and_return(nil)
.to receive(:perform_async).once.with(Integer, false).and_return(nil)
expect { run_service }
.to change { Event.count }.by(2)
......@@ -254,7 +273,7 @@ RSpec.describe DesignManagement::SaveDesignsService do
design_repository.has_visible_content?
expect(::DesignManagement::NewVersionWorker)
.to receive(:perform_async).once.with(Integer).and_return(nil)
.to receive(:perform_async).once.with(Integer, false).and_return(nil)
expect { service.execute }
.to change { issue.designs.count }.from(0).to(2)
......
......@@ -3,6 +3,8 @@
require 'spec_helper'
RSpec.describe Issues::MoveService do
include DesignManagementTestHelpers
let_it_be(:user) { create(:user) }
let_it_be(:author) { create(:user) }
let_it_be(:title) { 'Some issue' }
......@@ -201,6 +203,54 @@ RSpec.describe Issues::MoveService do
expect(copied_notes.order('id ASC').pluck(:note)).to eq(notes.map(&:note))
end
end
context 'issue with a design', :clean_gitlab_redis_shared_state do
let!(:design) { create(:design, :with_lfs_file, issue: old_issue) }
let!(:note) { create(:diff_note_on_design, noteable: design, issue: old_issue, project: old_issue.project) }
let(:subject) { move_service.execute(old_issue, new_project) }
before do
enable_design_management
end
it 'calls CopyDesignCollection::QueueService' do
expect(DesignManagement::CopyDesignCollection::QueueService).to receive(:new)
.with(user, old_issue, kind_of(Issue))
.and_call_original
subject
end
it 'logs if QueueService returns an error', :aggregate_failures do
error_message = 'error'
expect_next_instance_of(DesignManagement::CopyDesignCollection::QueueService) do |service|
expect(service).to receive(:execute).and_return(
ServiceResponse.error(message: error_message)
)
end
expect(Gitlab::AppLogger).to receive(:error).with(error_message)
subject
end
it 'does not call QueueService when the feature flag is disabled' do
stub_feature_flags(design_management_copy_designs: false)
expect(DesignManagement::CopyDesignCollection::QueueService).not_to receive(:new)
subject
end
# Perform a small integration test to ensure the services and worker
# can correctly create designs.
it 'copies the design and its notes', :sidekiq_inline, :aggregate_failures do
new_issue = subject
expect(new_issue.designs.size).to eq(1)
expect(new_issue.designs.first.notes.size).to eq(1)
end
end
end
describe 'move permissions' do
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe DesignManagement::CopyDesignCollectionWorker, :clean_gitlab_redis_shared_state do
describe '#perform' do
let_it_be(:user) { create(:user) }
let_it_be(:issue) { create(:issue) }
let_it_be(:target_issue) { create(:issue) }
subject { described_class.new.perform(user.id, issue.id, target_issue.id) }
it_behaves_like 'an idempotent worker' do
let(:job_args) { [user.id, issue.id, target_issue.id] }
specify { subject }
end
it 'calls DesignManagement::CopyDesignCollection::CopyService' do
expect_next_instance_of(DesignManagement::CopyDesignCollection::CopyService) do |service|
expect(service).to receive(:execute).and_return(ServiceResponse.success)
end
subject
end
it 'logs if there was an error calling the service' do
message = 'Error message'
allow_next_instance_of(DesignManagement::CopyDesignCollection::CopyService) do |service|
allow(service).to receive(:execute).and_return(ServiceResponse.error(message: message))
end
expect(Gitlab::AppLogger).to receive(:warn).with(message)
subject
end
end
end
......@@ -36,6 +36,10 @@ RSpec.describe DesignManagement::NewVersionWorker do
expect { worker.perform(version.id) }.to change { Note.system.count }.by(1)
end
it 'does not create a system note if skip_system_notes is true' do
expect { worker.perform(version.id, true) }.not_to change { Note.system.count }
end
it 'invokes GenerateImageVersionsService' do
expect_next_instance_of(DesignManagement::GenerateImageVersionsService) do |service|
expect(service).to receive(:execute)
......
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