From f133c608ff99b4598aa124611c23111e7f5b6f31 Mon Sep 17 00:00:00 2001
From: Nicolas Dular <ndular@gitlab.com>
Date: Thu, 23 May 2024 14:33:10 +0200
Subject: [PATCH] Use attributes for syncing work item to epic
Similar to syncing epics to work items, we now switch directly assigning
the AR attributes to the legacy epic. This makes it easier because:
1. We don't need to build up the parameters in the callbacks
2. We always sync all the data instead of partially syncing them based
on changed params.
---
app/services/work_items/callbacks/base.rb | 1 -
.../work_items/callbacks/description.rb | 5 -
.../callbacks/start_and_due_date.rb | 5 -
.../concerns/work_items/sync_as_epic.rb | 50 ++++-----
.../services/ee/work_items/callbacks/base.rb | 15 ---
.../ee/work_items/callbacks/description.rb | 21 ----
.../callbacks/start_and_due_date.rb | 20 ----
ee/app/services/work_items/callbacks/color.rb | 1 -
.../work_items/callbacks/description_spec.rb | 100 ------------------
.../callbacks/start_and_due_date_spec.rb | 61 -----------
.../work_items/callbacks/color_spec.rb | 12 ---
.../work_items/create_service_spec.rb | 12 +++
.../work_items/update_service_spec.rb | 14 ++-
13 files changed, 48 insertions(+), 269 deletions(-)
delete mode 100644 ee/app/services/ee/work_items/callbacks/base.rb
delete mode 100644 ee/app/services/ee/work_items/callbacks/description.rb
delete mode 100644 ee/app/services/ee/work_items/callbacks/start_and_due_date.rb
delete mode 100644 ee/spec/services/ee/work_items/callbacks/description_spec.rb
delete mode 100644 ee/spec/services/ee/work_items/callbacks/start_and_due_date_spec.rb
diff --git a/app/services/work_items/callbacks/base.rb b/app/services/work_items/callbacks/base.rb
index 97f1f0e26e4a..c91e2b37d108 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 fc94caf558c0..4a8e4d717698 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 713b8938a96f..76bfecce67f9 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 c362c5430ae1..ecbaa1403909 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 1d71b9d9ad3d..000000000000
--- 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 ab92b2264411..000000000000
--- 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 6093222876a8..000000000000
--- 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 27ccb7ba6f10..3bf3e6d0b7d9 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 77ffaa76947b..000000000000
--- 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 b48e7512f6ea..000000000000
--- 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 a6e2daae7cb5..c07b43478167 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 22ca1652d54d..35b5d75cb42e 100644
--- a/ee/spec/services/work_items/create_service_spec.rb
+++ b/ee/spec/services/work_items/create_service_spec.rb
@@ -157,6 +157,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 e999e35855b0..c10ae6139efb 100644
--- a/ee/spec/services/work_items/update_service_spec.rb
+++ b/ee/spec/services/work_items/update_service_spec.rb
@@ -436,6 +436,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
@@ -481,7 +493,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
--
GitLab