diff --git a/app/services/issuable/callbacks/base.rb b/app/services/issuable/callbacks/base.rb
index b697fb514808376dba85e38fe928bb0daa656b72..0ee412b112c47259a7b3629b66fbc061a48d31bc 100644
--- a/app/services/issuable/callbacks/base.rb
+++ b/app/services/issuable/callbacks/base.rb
@@ -13,6 +13,7 @@ def initialize(issuable:, current_user:, params: {})
end
def after_initialize; end
+ def before_create; end
def before_update; end
def after_update_commit; end
def after_save_commit; end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index c5554ed689b499251edfb785a42069ccb3b17a34..db570114cb669593a3373721531f6d1d58e808a0 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -254,6 +254,7 @@ def create(issuable, skip_system_notes: false)
before_create(issuable)
issuable_saved = issuable.with_transaction_returning_status do
+ @callbacks.each(&:before_create)
transaction_create(issuable)
end
diff --git a/doc/development/work_items_widgets.md b/doc/development/work_items_widgets.md
index 332dfde8a682be63c17f3e35225def69a7501f76..4579207530f6c1bd5c110fa2f54e48d977bfe829 100644
--- a/doc/development/work_items_widgets.md
+++ b/doc/development/work_items_widgets.md
@@ -162,6 +162,8 @@ legacy issue, merge request, or epic updates.
- `after_initialize` is called after the work item is initialized by the `BuildService` and before
the work item is saved by the `CreateService` and `UpdateService`. This callback runs outside the
creation or update database transaction.
+- `before_create` is called before the work item is saved by the `CreateService`. This callback runs
+ within the create database transaction.
- `before_update` is called before the work item is saved by the `UpdateService`. This callback runs
within the update database transaction.
- `after_update_commit` is called after the DB update transaction is committed by the `UpdateService`.
diff --git a/ee/app/services/concerns/work_items/sync_as_epic.rb b/ee/app/services/concerns/work_items/sync_as_epic.rb
index f461fb7609e1819da52f7a9b8a196f6cb05b95ed..807ce7ec3268a18a247c89dac23d030262486dda 100644
--- a/ee/app/services/concerns/work_items/sync_as_epic.rb
+++ b/ee/app/services/concerns/work_items/sync_as_epic.rb
@@ -6,6 +6,17 @@ module SyncAsEpic
private
+ def create_epic_for!(work_item)
+ return true unless work_item.namespace.work_item_sync_to_epic_enabled?
+
+ epic = Epic.create!(create_params(work_item))
+
+ work_item.relative_position = epic.id
+ work_item.save!(touch: false)
+ rescue StandardError => error
+ handle_error!(:create, error, work_item)
+ end
+
def update_epic_for!(work_item)
epic = work_item.synced_epic
return true unless epic
@@ -16,27 +27,51 @@ def update_epic_for!(work_item)
handle_error!(:update, error, work_item)
end
+ def create_params(work_item)
+ epic_params = {}
+
+ epic_params[:author] = work_item.author
+ epic_params[:group] = work_item.namespace
+ epic_params[:issue_id] = work_item.id
+ epic_params[:iid] = work_item.iid
+ epic_params[:created_at] = work_item.created_at
+
+ epic_params
+ .merge(callback_params)
+ .merge(base_attributes_params(work_item))
+ end
+
def update_params(work_item)
- epic_params = callback_params
+ callback_params
+ .merge(base_attributes_params(work_item))
+ end
+
+ def base_attributes_params(work_item)
+ base_params = {}
- epic_params[:confidential] = params[:confidential] if params.has_key?(:confidential)
- epic_params[:title] = params[:title] if params.has_key?(:title)
- epic_params[:title_html] = work_item.title_html if params.has_key?(:title)
- epic_params[:updated_by] = work_item.updated_by
- epic_params[:updated_at] = work_item.updated_at
- epic_params[:external_key] = params[:external_key] if params[:external_key]
+ if params.has_key?(:title)
+ base_params[:title] = params[:title]
+ base_params[:title_html] = work_item.title_html
+ end
+
+ base_params[:confidential] = params[:confidential] if params.has_key?(:confidential)
+ base_params[:updated_by] = work_item.updated_by
+ base_params[:updated_at] = work_item.updated_at
+ base_params[:external_key] = params[:external_key] if params[:external_key]
if work_item.edited?
- epic_params[:last_edited_at] = work_item.last_edited_at
- epic_params[:last_edited_by] = work_item.last_edited_by
+ base_params[:last_edited_at] = work_item.last_edited_at
+ base_params[:last_edited_by] = work_item.last_edited_by
end
- epic_params
+ base_params
end
def callback_params
callbacks.reduce({}) do |params, callback|
- params.merge!(callback.synced_epic_params) if callback.synced_epic_params.present?
+ next params unless callback.synced_epic_params.present?
+
+ params.merge!(callback.synced_epic_params)
end
end
@@ -45,10 +80,10 @@ def handle_error!(action, error, work_item)
message: "Not able to #{action} epic",
error_message: error.message,
group_id: work_item.namespace_id,
- work_item_id: work_item.id
+ work_item_id: work_item&.id
)
- ::Gitlab::ErrorTracking.track_and_raise_exception(error, work_item_id: work_item.id)
+ ::Gitlab::ErrorTracking.track_and_raise_exception(error, group_id: work_item.namespace_id)
end
end
end
diff --git a/ee/app/services/ee/work_items/create_service.rb b/ee/app/services/ee/work_items/create_service.rb
index b28a237508c9c67c50d7cfae86223e80ca5c618f..e80d7c9f03a364ce454117e415f2d0b05ff37e08 100644
--- a/ee/app/services/ee/work_items/create_service.rb
+++ b/ee/app/services/ee/work_items/create_service.rb
@@ -4,9 +4,23 @@ module EE
module WorkItems
module CreateService
extend ::Gitlab::Utils::Override
+ include ::WorkItems::SyncAsEpic
private
+ attr_reader :widget_params, :callbacks
+
+ override :transaction_create
+ def transaction_create(work_item)
+ return super unless work_item.epic_work_item?
+
+ super.tap do |save_result|
+ break save_result unless save_result
+
+ create_epic_for!(work_item)
+ end
+ end
+
override :iid_param_allowed?
def iid_param_allowed?
sync_work_item? || super
diff --git a/ee/app/services/work_items/callbacks/color.rb b/ee/app/services/work_items/callbacks/color.rb
index 94fa1ecb674a26362a4d900739bc4f3883a50952..27ccb7ba6f106b29f495f64a701b08b5f0de3fd2 100644
--- a/ee/app/services/work_items/callbacks/color.rb
+++ b/ee/app/services/work_items/callbacks/color.rb
@@ -5,10 +5,14 @@ module Callbacks
class Color < Base
ALLOWED_PARAMS = %i[color skip_system_notes].freeze
- def after_initialize
+ def before_create
+ handle_color_change unless excluded_in_new_type?
+ end
+
+ def before_update
return work_item.color.destroy! if work_item.color.present? && excluded_in_new_type?
- handle_color_change if params.key?(:color) && can_set_color?
+ handle_color_change
end
def after_save_commit
@@ -18,13 +22,17 @@ def after_save_commit
private
def handle_color_change
+ return unless params.key?(:color)
+ return unless can_set_color?
+
@previous_color = work_item&.color&.color&.to_s
color = work_item.color || work_item.build_color
return if params[:color] == color.color.to_s
color.color = params[:color]
- if color.save
+ if color.valid?
+ work_item.color = color
synced_epic_params[:color] = ::Gitlab::Color.of(params[:color])
else
raise_error(color.errors.full_messages.join(', '))
diff --git a/ee/spec/services/work_items/callbacks/color_spec.rb b/ee/spec/services/work_items/callbacks/color_spec.rb
index a7af16b6925f87d7eb65042cef2856750e2b55b2..a6e2daae7cb5b6f48e8a4242d590a3162389f073 100644
--- a/ee/spec/services/work_items/callbacks/color_spec.rb
+++ b/ee/spec/services/work_items/callbacks/color_spec.rb
@@ -7,7 +7,6 @@
let_it_be(:reporter) { create(:user) }
let_it_be(:group) { create(:group, reporters: reporter) }
let_it_be_with_reload(:work_item) { create(:work_item, :epic, namespace: group, author: user) }
- let_it_be_with_reload(:color) { create(:color, work_item: work_item, color: '#1068bf') }
let_it_be(:error_class) { ::WorkItems::Widgets::BaseService::WidgetError }
let(:current_user) { reporter }
@@ -18,105 +17,120 @@ def work_item_color
work_item.reload.color&.color.to_s
end
- describe '#after_initialize' do
- subject(:after_initialize_callback) { callback.after_initialize }
-
- shared_examples 'work item and color is unchanged' do
- it 'does not change work item color value' do
- expect { after_initialize_callback }
- .to not_change { work_item_color }
- .and not_change { work_item.updated_at }
- end
+ shared_examples 'work item and color is unchanged' do
+ it 'does not change work item color value' do
+ expect { subject }
+ .to not_change { work_item_color }
+ .and not_change { work_item.updated_at }
+ end
- it 'does not set synced_epic_params' do
- after_initialize_callback
+ it 'does not set synced_epic_params' do
+ subject
- expect(callback.synced_epic_params).to be_empty
- end
+ expect(callback.synced_epic_params).to be_empty
end
+ end
- shared_examples 'color is updated' do |color|
- it 'updates work item color value' do
- expect { after_initialize_callback }.to change { work_item_color }.to(color)
- end
+ shared_examples 'color is updated' do |color|
+ it 'updates work item color value' do
+ expect { subject }.to change { work_item_color }.to(color)
+ end
- it 'sets synced_epic_params' do
- after_initialize_callback
+ it 'sets synced_epic_params' do
+ subject
- expect(callback.synced_epic_params[:color].to_s).to eq(color)
- end
+ expect(callback.synced_epic_params[:color].to_s).to eq(color)
end
+ end
+
+ shared_examples 'raises a WidgetError' do
+ it { expect { subject }.to raise_error(error_class, message) }
+ end
- shared_examples 'raises a WidgetError' do
- it { expect { after_initialize_callback }.to raise_error(error_class, message) }
+ shared_examples 'when epic_colors feature is licensed' do
+ before do
+ stub_licensed_features(epic_colors: true)
end
- context 'when epic_colors feature is licensed' do
- before do
- stub_licensed_features(epic_colors: true)
+ context 'when color param is present' do
+ context 'when color param is valid' do
+ let(:params) { { color: '#454545' } }
+
+ it_behaves_like 'color is updated', '#454545'
end
+ end
- context 'when color param is present' do
- context 'when color param is valid' do
- let(:params) { { color: '#454545' } }
+ context 'when color param is not present' do
+ let(:params) { {} }
- it_behaves_like 'color is updated', '#454545'
- end
+ it_behaves_like 'work item and color is unchanged'
- context 'when widget does not exist in new type' do
- let(:params) { {} }
+ context 'when widget does not exist in type' do
+ let(:params) { {} }
- before do
- allow(callback).to receive(:excluded_in_new_type?).and_return(true)
- work_item.color = color
- end
+ before do
+ allow(callback).to receive(:excluded_in_new_type?).and_return(true)
+ end
- it "removes the work item's color" do
- expect { callback.after_initialize }.to change { work_item.reload.color }.from(color).to(nil)
- end
+ it "does not set the color" do
+ subject
+
+ expect(work_item.reload.color).to be_nil
end
end
+ end
- context 'when color param is not present' do
- let(:params) { {} }
+ context 'when color param is nil' do
+ let(:params) { { color: nil } }
- it_behaves_like 'work item and color is unchanged'
+ it_behaves_like 'raises a WidgetError' do
+ let(:message) { "Color can't be blank" }
end
+ end
- context 'when color is same as work item color' do
- let(:params) { { color: '#1068bf' } }
+ context 'when user cannot admin_work_item' do
+ let(:current_user) { user }
+ let(:params) { { color: '#1068bf' } }
- it_behaves_like 'work item and color is unchanged'
- end
+ it_behaves_like 'work item and color is unchanged'
+ end
+ end
- context 'when color param is nil' do
- let(:params) { { color: nil } }
+ shared_examples 'when epic_colors feature is unlicensed' do
+ before do
+ stub_licensed_features(epic_colors: false)
+ end
- it_behaves_like 'raises a WidgetError' do
- let(:message) { "Color can't be blank" }
- end
- end
+ it_behaves_like 'work item and color is unchanged'
+ end
- context 'when user cannot admin_work_item' do
- let(:current_user) { user }
- let(:params) { { color: '#1068bf' } }
+ describe '#before_update' do
+ subject(:before_update_callback) { callback.before_update }
- it_behaves_like 'work item and color is unchanged'
- end
- end
+ let_it_be_with_reload(:color) { create(:color, work_item: work_item, color: '#1068bf') }
- context 'when epic_colors feature is unlicensed' do
- before do
- stub_licensed_features(epic_colors: false)
- end
+ it_behaves_like 'when epic_colors feature is licensed'
+ it_behaves_like 'when epic_colors feature is unlicensed'
+
+ context 'when color is same as work item color' do
+ let(:params) { { color: '#1068bf' } }
it_behaves_like 'work item and color is unchanged'
end
end
+ describe '#before_create' do
+ subject(:before_create_callback) { callback.before_create }
+
+ it_behaves_like 'when epic_colors feature is licensed'
+ it_behaves_like 'when epic_colors feature is unlicensed'
+ end
+
describe '#after_save_commit' do
subject(:after_save_commit_callback) { callback.after_save_commit }
+ let_it_be_with_reload(:color) { create(:color, work_item: work_item, color: '#1068bf') }
+
it "does not create system notes when color didn't change" do
expect { after_save_commit_callback }.to not_change { work_item.notes.count }
end
diff --git a/ee/spec/services/work_items/create_service_spec.rb b/ee/spec/services/work_items/create_service_spec.rb
index baef0b744041539051560b829d2e78fa07c54a51..a48863bad0cb11f6c0c556f0df3e9a487544f083 100644
--- a/ee/spec/services/work_items/create_service_spec.rb
+++ b/ee/spec/services/work_items/create_service_spec.rb
@@ -107,4 +107,113 @@
it_behaves_like 'creates work item in container', :project
it_behaves_like 'creates work item in container', :project_namespace
it_behaves_like 'creates work item in container', :group
+
+ context 'for legacy epics' do
+ include_context 'with container for work items service', :group
+
+ let(:epic) { Epic.last }
+ let(:type) { WorkItems::Type.default_by_type(:epic) }
+
+ let(:start_date) { (Time.current + 1.day).to_date }
+ let(:due_date) { (Time.current + 2.days).to_date }
+
+ let(:widget_params) do
+ {
+ description_widget: {
+ description: 'new description'
+ },
+ color_widget: {
+ color: '#FF0000'
+ },
+ start_and_due_date_widget: { start_date: start_date, due_date: due_date }
+ }
+ end
+
+ let(:opts) { { title: 'new title', external_key: 'external_key', confidential: true, work_item_type: type } }
+ let(:current_user) { reporter }
+
+ before do
+ stub_licensed_features(epics: true, epic_colors: true)
+ stub_feature_flags(make_synced_work_item_read_only: false)
+ end
+
+ subject(:service_result) { service.execute }
+
+ it_behaves_like 'syncs all data from a work_item to an epic'
+
+ context 'when not creating an epic work item' do
+ let(:type) { WorkItems::Type.default_by_type(:task) }
+
+ it 'only creates a work item' do
+ expect { service_result }
+ .to not_change { Epic.count }
+ .and change { WorkItem.count }
+ end
+ end
+
+ context 'when creating the work item fails' do
+ before do
+ allow_next_instance_of(WorkItem) do |work_item|
+ allow(work_item).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new)
+ end
+ end
+
+ it 'does not update the epic or work item' do
+ expect(Gitlab::EpicWorkItemSync::Logger).to receive(:error)
+ .with({
+ message: "Not able to create epic",
+ error_message: "Record invalid",
+ group_id: group.id,
+ work_item_id: an_instance_of(Integer)
+ })
+
+ expect { service_result }
+ .to not_change { Epic.count }
+ .and not_change { WorkItem.count }
+ .and raise_error(ActiveRecord::RecordInvalid)
+ end
+ end
+
+ context 'when creating the epic fails' do
+ it 'does not create an epic or work item' do
+ allow(Epic).to receive(:create!).and_raise(ActiveRecord::RecordInvalid.new)
+
+ expect(Gitlab::EpicWorkItemSync::Logger).to receive(:error)
+ .with({
+ message: "Not able to create epic",
+ error_message: "Record invalid",
+ group_id: group.id,
+ work_item_id: an_instance_of(Integer)
+ })
+
+ expect { service_result }
+ .to not_change { WorkItem.count }
+ .and not_change { Epic.count }
+ .and raise_error(ActiveRecord::RecordInvalid)
+ end
+ end
+
+ context 'when changes are invalid' do
+ let(:widget_params) { {} }
+ let(:opts) { { title: '' } }
+
+ it 'does not create an epic or work item' do
+ expect { service_result }
+ .to not_change { WorkItem.count }
+ .and not_change { Epic.count }
+ end
+ end
+
+ context 'when feature flag is disabled' do
+ before do
+ stub_feature_flags(make_synced_work_item_read_only: false, sync_work_item_to_epic: false)
+ end
+
+ it 'only creates a work item but not the epic' do
+ expect { service_result }
+ .to change { WorkItem.count }
+ .and not_change { Epic.count }
+ end
+ end
+ end
end
diff --git a/ee/spec/services/work_items/update_service_spec.rb b/ee/spec/services/work_items/update_service_spec.rb
index 545c631a7d0bc99daa5217e46d232b8cfb1ee1a9..e999e35855b0b3e680441866ea9093933d137b50 100644
--- a/ee/spec/services/work_items/update_service_spec.rb
+++ b/ee/spec/services/work_items/update_service_spec.rb
@@ -394,6 +394,15 @@
let(:start_date) { (Time.current + 1.day).to_date }
let(:due_date) { (Time.current + 2.days).to_date }
+ let(:service) do
+ described_class.new(
+ container: group,
+ current_user: current_user,
+ params: params,
+ widget_params: widget_params
+ )
+ end
+
let(:widget_params) do
{
description_widget: {