diff --git a/app/services/work_items/parent_links/reorder_service.rb b/app/services/work_items/parent_links/reorder_service.rb
index 8422df030f52c43f38483344bdf3635e16c38a11..dbbc6860ed791eac9e16d3c4ee7dc67fd39da68a 100644
--- a/app/services/work_items/parent_links/reorder_service.rb
+++ b/app/services/work_items/parent_links/reorder_service.rb
@@ -9,9 +9,10 @@ class ReorderService < WorkItems::ParentLinks::BaseService
def relate_issuables(work_item)
notes_are_expected = work_item.work_item_parent != issuable
link = set_parent(issuable, work_item)
- reorder(link, params[:adjacent_work_item], params[:relative_position])
- create_notes(work_item) if link.save && notes_are_expected
+ if reorder(link, params[:adjacent_work_item], params[:relative_position]) && notes_are_expected
+ create_notes(work_item)
+ end
link
end
@@ -19,18 +20,14 @@ def relate_issuables(work_item)
def reorder(link, adjacent_work_item, relative_position)
WorkItems::ParentLink.move_nulls_to_end(RelativePositioning.mover.context(link).relative_siblings)
- adjacent_parent_link = adjacent_work_item.parent_link
- # if issuable is an epic, we can create the missing parent link between epic work item and adjacent_work_item
- if adjacent_parent_link.blank? && adjacent_work_item.synced_epic
- adjacent_parent_link = set_parent(issuable, adjacent_work_item)
- adjacent_parent_link.relative_position = adjacent_work_item.synced_epic.relative_position
- adjacent_parent_link.save!
- end
-
- return unless adjacent_parent_link
+ move_link(link, adjacent_work_item, relative_position)
+ end
- link.move_before(adjacent_parent_link) if relative_position == 'BEFORE'
- link.move_after(adjacent_parent_link) if relative_position == 'AFTER'
+ # overriden in EE
+ def move_link(link, adjacent_work_item, relative_position)
+ link.move_before(adjacent_work_item.parent_link) if relative_position == 'BEFORE'
+ link.move_after(adjacent_work_item.parent_link) if relative_position == 'AFTER'
+ link.save
end
override :render_conflict_error?
diff --git a/ee/app/services/ee/work_items/parent_links/reorder_service.rb b/ee/app/services/ee/work_items/parent_links/reorder_service.rb
index 1240c8ed0a54d82b79b1cad6cffd5e823dad3555..541ac5959dbb6486ad8370ba77237625ba6819b5 100644
--- a/ee/app/services/ee/work_items/parent_links/reorder_service.rb
+++ b/ee/app/services/ee/work_items/parent_links/reorder_service.rb
@@ -7,8 +7,78 @@ module ReorderService
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
+ def execute
+ super
+ rescue ::WorkItems::SyncAsEpic::SyncAsEpicError => _error
+ error(_("Couldn't re-order due to an internal error."), 422)
+ end
+
private
+ override :move_link
+ def move_link(link, adjacent_work_item, relative_position)
+ create_missing_synced_link!(adjacent_work_item)
+ return unless adjacent_work_item.parent_link
+
+ return super unless sync_to_epic?(link, adjacent_work_item)
+
+ ApplicationRecord.transaction do
+ move_synced_object!(link, adjacent_work_item, relative_position) if super
+ end
+ end
+
+ def create_missing_synced_link!(adjacent_work_item)
+ adjacent_parent_link = adjacent_work_item.parent_link
+ # if issuable is an epic, we can create the missing parent link between epic work item and adjacent_work_item
+ return unless adjacent_parent_link.blank? && adjacent_work_item.synced_epic
+
+ adjacent_parent_link = set_parent(issuable, adjacent_work_item)
+ adjacent_parent_link.relative_position = adjacent_work_item.synced_epic.relative_position
+ adjacent_parent_link.save!
+
+ # we update the adjacent_work_item's parent link but use the adjacent_work_item object.
+ adjacent_work_item.reset
+ end
+
+ def move_synced_object!(link, adjacent_work_item, relative_position)
+ synced_moving_object = synced_object_for(link.work_item)
+ synced_adjacent_object = synced_object_for(adjacent_work_item)
+
+ synced_moving_object.move_before(synced_adjacent_object) if relative_position == 'BEFORE'
+ synced_moving_object.move_after(synced_adjacent_object) if relative_position == 'AFTER'
+
+ synced_moving_object.save!(touch: false)
+ rescue StandardError => error
+ ::Gitlab::EpicWorkItemSync::Logger.error(
+ message: "Not able to sync re-ordering work item",
+ error_message: error.message,
+ namespace_id: issuable.namespace_id,
+ synced_moving_object_id: synced_moving_object.id,
+ synced_moving_object_class: synced_moving_object.class
+ )
+
+ ::Gitlab::ErrorTracking.track_exception(error, namespace_id: issuable.namespace_id)
+
+ raise ::WorkItems::SyncAsEpic::SyncAsEpicError
+ end
+
+ def synced_object_for(work_item)
+ case work_item.synced_epic
+ when nil
+ ::EpicIssue.find_by_issue_id(work_item.id)
+ when ::Epic
+ work_item.synced_epic
+ end
+ end
+
+ def sync_to_epic?(link, adjacent_work_item)
+ return false if synced_work_item
+ return false if link.work_item_parent.synced_epic.nil?
+ return false unless link.work_item.namespace.root_ancestor.work_item_sync_to_epic_enabled?
+
+ synced_object_for(link.work_item) && synced_object_for(adjacent_work_item)
+ end
+
override :can_admin_link?
def can_admin_link?(work_item)
return true if synced_work_item
diff --git a/ee/spec/services/ee/work_items/parent_links/reorder_service_spec.rb b/ee/spec/services/ee/work_items/parent_links/reorder_service_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..e88b83a9ee09c3b9f9d220cbaa5fe4c545b43929
--- /dev/null
+++ b/ee/spec/services/ee/work_items/parent_links/reorder_service_spec.rb
@@ -0,0 +1,254 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe WorkItems::ParentLinks::ReorderService, feature_category: :portfolio_management do
+ describe '#execute' do
+ let_it_be(:group) { create(:group) }
+ let_it_be(:user) { create(:user, developer_of: group) }
+
+ let_it_be_with_reload(:parent) { create(:work_item, :epic_with_legacy_epic, namespace: group) }
+ let_it_be_with_reload(:top_adjacent) { create(:work_item, :epic_with_legacy_epic, namespace: group) }
+ let_it_be_with_reload(:last_adjacent) { create(:work_item, :epic_with_legacy_epic, namespace: group) }
+ let_it_be_with_reload(:work_item) { create(:work_item, :epic_with_legacy_epic, namespace: group) }
+
+ let(:relative_position) { 'AFTER' }
+ let(:adjacent_work_item) { top_adjacent }
+ let(:base_params) { { target_issuable: work_item, relative_position: relative_position } }
+ let(:params) { base_params.merge(adjacent_work_item: adjacent_work_item) }
+
+ let(:work_item_position) { work_item.parent_link.reload.relative_position }
+ let(:top_adjacent_position) { top_adjacent.parent_link.reload.relative_position }
+ let(:last_adjacent_position) { last_adjacent.parent_link.reload.relative_position }
+ let(:synced_moving_object) { work_item.synced_epic.reload }
+
+ subject(:execute) { described_class.new(parent, user, params).execute }
+
+ shared_examples 'reorders the hierarchy' do
+ context 'when relative_position is AFTER' do
+ let(:relative_position) { 'AFTER' }
+
+ it 'reorders correctly' do
+ subject
+
+ expect(work_item_position).to be > top_adjacent_position
+ expect(work_item_position).to be < last_adjacent_position
+
+ if synced_moving_object
+ expect(synced_moving_object.relative_position).to be > top_adjacent_position
+ expect(synced_moving_object.relative_position).to be < last_adjacent_position
+ end
+ end
+ end
+
+ context 'when relative_position is BEFORE' do
+ let(:relative_position) { 'BEFORE' }
+
+ it 'reorders correctly' do
+ subject
+
+ expect(work_item_position).to be < top_adjacent_position
+ expect(work_item_position).to be < last_adjacent_position
+
+ if synced_moving_object
+ expect(synced_moving_object.relative_position).to be < top_adjacent_position
+ expect(synced_moving_object.relative_position).to be < last_adjacent_position
+ end
+ end
+ end
+ end
+
+ shared_examples 'only changes work item' do
+ it 'does not change synced moving object relative position but work item one' do
+ expect { subject }.to not_change { synced_moving_object.reload.relative_position }
+ .and change { work_item.parent_link.reload.relative_position }
+ end
+ end
+
+ shared_examples 'when saving fails' do |failing_class, expect_error_log: false, expect_error_message: false|
+ it "does not change any position when saving #{failing_class}" do
+ allow_next_found_instance_of(failing_class) do |instance|
+ # Epic and EpicIssue are saved with !
+ allow(instance).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new)
+ # WorkItem::ParentLink is saved without a !
+ allow(instance).to receive(:save).and_return(false)
+ end
+
+ if expect_error_log
+ expect(Gitlab::EpicWorkItemSync::Logger).to receive(:error)
+ .with({
+ message: "Not able to sync re-ordering work item",
+ error_message: 'Record invalid',
+ namespace_id: group.id,
+ synced_moving_object_id: synced_moving_object.id,
+ synced_moving_object_class: synced_moving_object.class
+ })
+
+ end
+
+ expect { execute }.to not_change { work_item.parent_link.reload.relative_position }
+ .and not_change { synced_moving_object.reload.relative_position }
+
+ if expect_error_message
+ expect(execute)
+ .to eq({ status: :error, message: "Couldn't re-order due to an internal error.", http_status: 422 })
+ end
+ end
+ end
+
+ before do
+ stub_feature_flags(make_synced_work_item_read_only: false)
+ end
+
+ context 'when adjacent_work_item parent link is missing' do
+ let(:synced_moving_object) { nil }
+
+ before do
+ create(:parent_link, work_item: work_item, work_item_parent: parent)
+ end
+
+ context 'when adjacent work item has a synced epic' do
+ it 'creates a new parent link' do
+ expect(top_adjacent.reload.parent_link).to be_nil
+ expect { execute }.to change { ::WorkItems::ParentLink.count }.by(1)
+ expect(top_adjacent.reload.parent_link).to be_present
+ end
+ end
+
+ context 'when adjacent work item has no synced epic' do
+ let_it_be_with_reload(:top_adjacent) { create(:work_item, :epic, namespace: group) }
+
+ it 'does not create a new parent link' do
+ expect { execute }.not_to change { WorkItems::ParentLink.count }
+ end
+ end
+ end
+
+ context 'when moving an epic work item' do
+ let(:synced_moving_object) { work_item.synced_epic.reload }
+
+ let_it_be_with_reload(:top_adjacent_link) do
+ create(:parent_link, work_item: top_adjacent, work_item_parent: parent, relative_position: 20)
+ end
+
+ let_it_be_with_reload(:last_adjacent_link) do
+ create(:parent_link, work_item: last_adjacent, work_item_parent: parent, relative_position: 30)
+ end
+
+ let_it_be_with_reload(:work_item_link) do
+ create(:parent_link, work_item: work_item, work_item_parent: parent, relative_position: 40)
+ end
+
+ context 'when synced epics for the work items exist' do
+ before do
+ top_adjacent.synced_epic.update!(parent: parent.synced_epic, relative_position: 20)
+ last_adjacent.synced_epic.update!(parent: parent.synced_epic, relative_position: 30)
+ work_item.synced_epic.update!(parent: parent.synced_epic, relative_position: 40)
+ end
+
+ it_behaves_like 'reorders the hierarchy'
+
+ context 'when synced_work_item param is set' do
+ let(:synced_moving_object) { nil }
+ let(:params) { base_params.merge(adjacent_work_item: adjacent_work_item, synced_work_item: true) }
+
+ it_behaves_like 'reorders the hierarchy'
+ it_behaves_like 'only changes work item' do
+ let(:synced_moving_object) { work_item.synced_epic.reload }
+ end
+ end
+
+ context 'when sync_work_item_to_epic feature flag is disabled' do
+ before do
+ stub_feature_flags(sync_work_item_to_epic: false, make_synced_work_item_read_only: false)
+ end
+
+ it_behaves_like 'only changes work item'
+ end
+ end
+
+ context 'when synced epic for the moving work item do not exist' do
+ let(:synced_moving_object) { nil }
+ let_it_be_with_reload(:work_item) { create(:work_item, :epic, namespace: group) }
+
+ it_behaves_like 'reorders the hierarchy'
+ end
+
+ context 'when synced epics for the parent epic do not exist' do
+ let_it_be_with_reload(:parent) { create(:work_item, :epic, namespace: group) }
+
+ it_behaves_like 'only changes work item'
+ end
+
+ context 'when synced epic for the adjacent work item does not exist' do
+ let(:synced_moving_object) { nil }
+
+ before do
+ top_adjacent.synced_epic.update!(issue_id: nil)
+ end
+
+ it_behaves_like 'reorders the hierarchy'
+ end
+
+ it_behaves_like 'when saving fails', Epic, expect_error_log: true, expect_error_message: true
+ it_behaves_like 'when saving fails', WorkItems::ParentLink
+ end
+
+ context 'when moving an issue work item' do
+ let(:synced_moving_object) { epic_issue.reload }
+
+ let_it_be_with_reload(:work_item) { create(:work_item, :issue, namespace: group) }
+
+ let_it_be(:epic_issue) do
+ create(:epic_issue, epic: parent.synced_epic, issue: Issue.find(work_item.id), relative_position: 40)
+ end
+
+ let_it_be_with_reload(:top_adjacent_link) do
+ create(:parent_link, work_item: top_adjacent, work_item_parent: parent, relative_position: 20)
+ end
+
+ let_it_be_with_reload(:last_adjacent_link) do
+ create(:parent_link, work_item: last_adjacent, work_item_parent: parent, relative_position: 30)
+ end
+
+ let_it_be_with_reload(:work_item_link) do
+ create(:parent_link, work_item: work_item, work_item_parent: parent, relative_position: 40)
+ end
+
+ before do
+ top_adjacent.synced_epic.update!(parent: parent.synced_epic, relative_position: 20)
+ last_adjacent.synced_epic.update!(parent: parent.synced_epic, relative_position: 30)
+ end
+
+ it_behaves_like 'reorders the hierarchy'
+
+ context 'when sync_work_item_to_epic feature flag is disabled' do
+ before do
+ stub_feature_flags(sync_work_item_to_epic: false, make_synced_work_item_read_only: false)
+ end
+
+ it_behaves_like 'only changes work item'
+ end
+
+ context 'when synced EpicIssue for the moving work item do not exist' do
+ let(:synced_moving_object) { nil }
+ let_it_be_with_reload(:epic_issue) { nil }
+
+ it_behaves_like 'reorders the hierarchy'
+ end
+
+ context 'when synced epic for the adjacent work item does not exist' do
+ let(:synced_moving_object) { nil }
+
+ before do
+ top_adjacent.synced_epic.update!(issue_id: nil)
+ end
+
+ it_behaves_like 'reorders the hierarchy'
+ end
+
+ it_behaves_like 'when saving fails', EpicIssue, expect_error_log: true, expect_error_message: true
+ it_behaves_like 'when saving fails', WorkItems::ParentLink
+ end
+ end
+end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 1d724f58e9c7a2557d1441d09d8d443915db4640..f95fa850d9bacb0bb2d2c8d4ac7e5c363a9cab96 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -15035,6 +15035,9 @@ msgstr ""
msgid "Couldn't link issues. You must have at least the Guest role in both projects."
msgstr ""
+msgid "Couldn't re-order due to an internal error."
+msgstr ""
+
msgid "Couldn't reorder child due to an internal error."
msgstr ""