diff --git a/app/services/work_items/callbacks/base.rb b/app/services/work_items/callbacks/base.rb index 97f1f0e26e4aa48af7c6bb9d9757e282a5c18dea..c91e2b37d108016f47fc0935e6b6337e77ab8393 100644 --- a/app/services/work_items/callbacks/base.rb +++ b/app/services/work_items/callbacks/base.rb @@ -11,4 +11,3 @@ def raise_error(message) end end end -WorkItems::Callbacks::Base.prepend_mod diff --git a/app/services/work_items/callbacks/description.rb b/app/services/work_items/callbacks/description.rb index fc94caf558c0a4db62e05e1094f5ad6f7818e35c..4a8e4d7176983a8750612ff28d80ca79dc839bb7 100644 --- a/app/services/work_items/callbacks/description.rb +++ b/app/services/work_items/callbacks/description.rb @@ -3,8 +3,6 @@ module WorkItems module Callbacks class Description < Base - include Gitlab::Utils::StrongMemoize - def after_initialize params[:description] = nil if excluded_in_new_type? @@ -19,9 +17,6 @@ def after_initialize def update_description? params.present? && params.key?(:description) && has_permission?(:update_work_item) end - strong_memoize_attr :update_description? end end end - -WorkItems::Callbacks::Description.prepend_mod diff --git a/app/services/work_items/callbacks/start_and_due_date.rb b/app/services/work_items/callbacks/start_and_due_date.rb index 713b8938a96f0988c23bfc3d65781ff597714099..76bfecce67f9620c7af518bb6bc89256461fd97c 100644 --- a/app/services/work_items/callbacks/start_and_due_date.rb +++ b/app/services/work_items/callbacks/start_and_due_date.rb @@ -3,8 +3,6 @@ module WorkItems module Callbacks class StartAndDueDate < Base - include Gitlab::Utils::StrongMemoize - def before_update return work_item.assign_attributes({ start_date: nil, due_date: nil }) if excluded_in_new_type? @@ -18,9 +16,6 @@ def before_update def update_start_and_due_date? params.present? && has_permission?(:set_work_item_metadata) end - strong_memoize_attr :update_start_and_due_date? end end end - -WorkItems::Callbacks::StartAndDueDate.prepend_mod 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 c362c5430ae14be153802aef874d3c1c3f43c640..ecbaa14039097da64f2085d1df9dad4a1d27381f 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,12 @@ module SyncAsEpic private + BASE_ATTRIBUTE_PARAMS = %i[ + iid author_id created_at updated_at title title_html description description_html + confidential state_id last_edited_by_id last_edited_at external_key updated_by_id + closed_at closed_by_id imported_from + ].freeze + def create_epic_for!(work_item) return true unless work_item.namespace.work_item_sync_to_epic_enabled? @@ -22,7 +28,8 @@ def update_epic_for!(work_item) return true unless epic return true unless epic.group.work_item_sync_to_epic_enabled? - epic.update!(update_params(work_item)) + epic.assign_attributes(update_params(work_item)) + epic.save!(touch: false) rescue StandardError => error handle_error!(:update, error, work_item) end @@ -30,11 +37,9 @@ def update_epic_for!(work_item) 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 parent_link = WorkItems::ParentLink.find_by_work_item_id(work_item.id) @@ -44,42 +49,33 @@ def create_params(work_item) end epic_params - .merge(callback_params) .merge(base_attributes_params(work_item)) + .merge(color_params(work_item)) + .merge(date_params(work_item)) end def update_params(work_item) - callback_params + {} .merge(base_attributes_params(work_item)) + .merge(color_params(work_item)) + .merge(date_params(work_item)) end def base_attributes_params(work_item) - base_params = {} - - 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] + BASE_ATTRIBUTE_PARAMS.index_with { |attr| work_item[attr] } + end - if work_item.edited? - base_params[:last_edited_at] = work_item.last_edited_at - base_params[:last_edited_by] = work_item.last_edited_by - end + def color_params(work_item) + return {} unless work_item.color - base_params + { color: work_item.color.color } end - def callback_params - callbacks.reduce({}) do |params, callback| - next params unless callback.synced_epic_params.present? - - params.merge!(callback.synced_epic_params) - end + def date_params(work_item) + { + start_date: work_item.start_date, + due_date: work_item.due_date + } end def handle_error!(action, error, work_item) diff --git a/ee/app/services/ee/work_items/callbacks/base.rb b/ee/app/services/ee/work_items/callbacks/base.rb deleted file mode 100644 index 1d71b9d9ad3d77d7192e3fd075432c7ae3451281..0000000000000000000000000000000000000000 --- a/ee/app/services/ee/work_items/callbacks/base.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module EE - module WorkItems - module Callbacks - module Base - extend ::Gitlab::Utils::Override - - def synced_epic_params - @synced_epic_params ||= {} - end - end - end - end -end diff --git a/ee/app/services/ee/work_items/callbacks/description.rb b/ee/app/services/ee/work_items/callbacks/description.rb deleted file mode 100644 index ab92b226441128e701556dbe91d86af9bb5c5fef..0000000000000000000000000000000000000000 --- a/ee/app/services/ee/work_items/callbacks/description.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -module EE - module WorkItems - module Callbacks - module Description - extend ::Gitlab::Utils::Override - - override :after_initialize - def after_initialize - super - - return unless update_description? - - synced_epic_params[:description] = params[:description] - synced_epic_params[:description_html] = work_item.description_html - end - end - end - end -end diff --git a/ee/app/services/ee/work_items/callbacks/start_and_due_date.rb b/ee/app/services/ee/work_items/callbacks/start_and_due_date.rb deleted file mode 100644 index 6093222876a8cbf5a101e6dd190489064c6c9b02..0000000000000000000000000000000000000000 --- a/ee/app/services/ee/work_items/callbacks/start_and_due_date.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true - -module EE - module WorkItems - module Callbacks - module StartAndDueDate - extend ::Gitlab::Utils::Override - - override :before_update - def before_update - super - return unless update_start_and_due_date? - - synced_epic_params[:start_date] = params[:start_date] - synced_epic_params[:due_date] = params[:due_date] - end - end - end - end -end diff --git a/ee/app/services/work_items/callbacks/color.rb b/ee/app/services/work_items/callbacks/color.rb index 27ccb7ba6f106b29f495f64a701b08b5f0de3fd2..3bf3e6d0b7d93abe9fcbaa6ead5b11d52fc4e8c1 100644 --- a/ee/app/services/work_items/callbacks/color.rb +++ b/ee/app/services/work_items/callbacks/color.rb @@ -33,7 +33,6 @@ def handle_color_change if color.valid? work_item.color = color - synced_epic_params[:color] = ::Gitlab::Color.of(params[:color]) else raise_error(color.errors.full_messages.join(', ')) end diff --git a/ee/spec/services/ee/work_items/callbacks/description_spec.rb b/ee/spec/services/ee/work_items/callbacks/description_spec.rb deleted file mode 100644 index 77ffaa76947b60a29bb95c1a303bb09c9a235e9d..0000000000000000000000000000000000000000 --- a/ee/spec/services/ee/work_items/callbacks/description_spec.rb +++ /dev/null @@ -1,100 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe WorkItems::Callbacks::Description, feature_category: :portfolio_management do - let_it_be(:random_user) { create(:user) } - let_it_be(:author) { create(:user) } - let_it_be(:project) { create(:project, :public) } - let_it_be(:guest) { create(:user, guest_of: project) } - let_it_be(:reporter) { create(:user, reporter_of: project) } - - let(:params) { { description: 'updated description' } } - let(:current_user) { author } - let(:work_item) do - create( - :work_item, - author: author, - project: project, - description: 'old description', - last_edited_at: Date.yesterday, - last_edited_by: random_user - ) - end - - describe '#after_initialize' do - let(:service) { described_class.new(issuable: work_item, current_user: current_user, params: params) } - - subject(:after_initialize_callback) { service.after_initialize } - - shared_examples 'sets synced_epic_params' do - it 'set the synced_epic_params' do - subject - - expect(service.synced_epic_params[:description]).to eq(params[:description]) - expect(service.synced_epic_params[:description_html]).to eq(work_item.description_html) - end - end - - shared_examples 'does not set synced_epic_params' do - it 'does not set the synced_epic_params' do - subject - - expect(service.synced_epic_params[:description]).to be_nil - expect(service.synced_epic_params[:description_html]).to be_nil - end - end - - context 'when user has permission to update description' do - context 'when user is work item author' do - let(:current_user) { author } - - it_behaves_like 'sets synced_epic_params' - end - - context 'when user is a project reporter' do - let(:current_user) { reporter } - - it_behaves_like 'sets synced_epic_params' - end - - context 'when description is nil' do - let(:current_user) { author } - let(:params) { { description: nil } } - - it_behaves_like 'sets synced_epic_params' - end - - context 'when description is empty' do - let(:current_user) { author } - let(:params) { { description: '' } } - - it_behaves_like 'sets synced_epic_params' - end - - context 'when description param is not present' do - let(:params) { {} } - - it_behaves_like 'does not set synced_epic_params' - end - end - - context 'when user does not have permission to update description' do - context 'when user is a project guest' do - let(:current_user) { guest } - - it_behaves_like 'does not set synced_epic_params' - end - - context 'with private project' do - let_it_be(:project) { create(:project) } - - context 'when user is work item author' do - let(:current_user) { author } - - it_behaves_like 'does not set synced_epic_params' - end - end - end - end -end diff --git a/ee/spec/services/ee/work_items/callbacks/start_and_due_date_spec.rb b/ee/spec/services/ee/work_items/callbacks/start_and_due_date_spec.rb deleted file mode 100644 index b48e7512f6ea17485d89d481b8ab42189fa19864..0000000000000000000000000000000000000000 --- a/ee/spec/services/ee/work_items/callbacks/start_and_due_date_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe WorkItems::Callbacks::StartAndDueDate, feature_category: :portfolio_management do - let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user, reporter_of: project) } - let_it_be_with_reload(:work_item) { create(:work_item, project: project) } - - let(:widget) { work_item.widgets.find { |widget| widget.is_a?(WorkItems::Callbacks::StartAndDueDate) } } - - describe '#before_update_callback' do - let(:start_date) { Date.today } - let(:due_date) { 1.week.from_now.to_date } - let(:service) { described_class.new(issuable: work_item, current_user: user, params: params) } - - subject(:update_params) { service.before_update } - - shared_examples 'does not set synced_epic_params' do - it 'does not set synced_epic_params' do - update_params - - expect(service.synced_epic_params).to be_empty - end - end - - context 'when start and due date params are present' do - let(:params) { { start_date: Date.today, due_date: 1.week.from_now.to_date } } - - it 'correctly sets synced_epic_params' do - update_params - - expect(service.synced_epic_params[:start_date]).to eq(start_date) - expect(service.synced_epic_params[:due_date]).to eq(due_date) - end - - context "and user doesn't have permissions to update start and due date" do - let_it_be(:user) { create(:user) } - - it_behaves_like 'does not set synced_epic_params' - end - end - - context 'when date params are not present' do - let(:params) { {} } - - it_behaves_like 'does not set synced_epic_params' - end - - context 'when start_date and due_date are null' do - context 'when one of the two params is null' do - let(:params) { { start_date: nil, due_date: nil } } - - it 'sets only one date to null' do - expect(service.synced_epic_params[:start_date]).to eq(nil) - expect(service.synced_epic_params[:due_date]).to eq(nil) - end - end - end - end -end diff --git a/ee/spec/services/work_items/callbacks/color_spec.rb b/ee/spec/services/work_items/callbacks/color_spec.rb index a6e2daae7cb5b6f48e8a4242d590a3162389f073..c07b43478167570505ffb204bcafa25a74de3090 100644 --- a/ee/spec/services/work_items/callbacks/color_spec.rb +++ b/ee/spec/services/work_items/callbacks/color_spec.rb @@ -23,24 +23,12 @@ def work_item_color .to not_change { work_item_color } .and not_change { work_item.updated_at } end - - it 'does not set synced_epic_params' do - subject - - 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 { subject }.to change { work_item_color }.to(color) end - - it 'sets synced_epic_params' do - subject - - expect(callback.synced_epic_params[:color].to_s).to eq(color) - end end shared_examples 'raises a WidgetError' do diff --git a/ee/spec/services/work_items/create_service_spec.rb b/ee/spec/services/work_items/create_service_spec.rb index 52b76a86a9a4919712f1cfea5e13383397e96c74..de60cdbfa8a5dd67930bc0f2912c70d3543ac023 100644 --- a/ee/spec/services/work_items/create_service_spec.rb +++ b/ee/spec/services/work_items/create_service_spec.rb @@ -136,6 +136,18 @@ it_behaves_like 'syncs all data from a work_item to an epic' + context 'when creating the epic with only title and description' do + let(:widget_params) do + { + description_widget: { + description: 'new description' + } + } + end + + it_behaves_like 'syncs all data from a work_item to an epic' + end + context 'when creating an epic work item' do it 'creates the epic with correct relative_position' do work_item = service_result.payload[:work_item] diff --git a/ee/spec/services/work_items/update_service_spec.rb b/ee/spec/services/work_items/update_service_spec.rb index 5ccd1398ddbb5d770360b8862c23b9ec2f3f5716..45e3c2cf0f382f11a53f8c5a763b34f1d157dafd 100644 --- a/ee/spec/services/work_items/update_service_spec.rb +++ b/ee/spec/services/work_items/update_service_spec.rb @@ -326,6 +326,18 @@ it_behaves_like 'syncs all data from a work_item to an epic' + context 'when only providing title and description' do + let(:widget_params) do + { + description_widget: { + description: 'new description' + } + } + end + + it_behaves_like 'syncs all data from a work_item to an epic' + end + it 'syncs the data to the epic', :aggregate_failures do update_work_item @@ -371,7 +383,7 @@ context 'when updating the epic fails' do before do allow_next_found_instance_of(Epic) do |epic| - allow(epic).to receive(:update!).and_raise(ActiveRecord::RecordInvalid.new) + allow(epic).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new) end end