From da62cfe7fbad252cde98b7c66244d9cbdcbade9c Mon Sep 17 00:00:00 2001
From: Eugenia Grieff <egrieff@gitlab.com>
Date: Fri, 19 Apr 2024 16:32:38 +0000
Subject: [PATCH] Sync epic child reordering with associated work item
The a child epic is reordered the corresponding work item parent link should
match its position
---
ee/app/models/ee/group.rb | 5 +
.../epics/epic_links/update_service.rb | 41 +++-
.../epics/epic_links_controller_spec.rb | 4 +-
.../epics/epic_links/update_service_spec.rb | 175 ++++++++++++++----
locale/gitlab.pot | 3 +
5 files changed, 189 insertions(+), 39 deletions(-)
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 2fdca22c7610b..475277ad7f6ac 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -257,6 +257,11 @@ def epic_sync_to_work_item_enabled?
def work_item_sync_to_epic_enabled?
::Feature.enabled?(:sync_work_item_to_epic, self, type: :wip)
end
+
+ def sync_epic_work_item_order_enabled?
+ epic_sync_to_work_item_enabled? &&
+ ::Feature.enabled?(:sync_epic_work_item_order, self, type: :gitlab_com_derisk)
+ end
end
override :supports_saved_replies?
diff --git a/ee/app/services/epics/epic_links/update_service.rb b/ee/app/services/epics/epic_links/update_service.rb
index 0a9068c201931..89e63c47db4c0 100644
--- a/ee/app/services/epics/epic_links/update_service.rb
+++ b/ee/app/services/epics/epic_links/update_service.rb
@@ -19,8 +19,18 @@ def execute
move_epic!
success
- rescue ActiveRecord::RecordNotFound
- error('Epic not found for given params', 404)
+ rescue ActiveRecord::RecordNotFound, ::Epics::SyncAsWorkItem::SyncAsWorkItemError => e
+ message =
+ if e.is_a?(ActiveRecord::RecordNotFound)
+ _('Epic not found for given params')
+ else
+ Gitlab::ErrorTracking.track_exception(e, moving_epic_id: epic.id)
+ Gitlab::EpicWorkItemSync::Logger.error(error: e)
+
+ _("Couldn't reorder child due to an internal error.")
+ end
+
+ error(message, 422)
end
private
@@ -31,8 +41,31 @@ def move_epic!
before_epic = Epic.in_parents(epic.parent_id).find(params[:move_before_id]) if params[:move_before_id]
after_epic = Epic.in_parents(epic.parent_id).find(params[:move_after_id]) if params[:move_after_id]
- epic.move_between(before_epic, after_epic)
- epic.save!(touch: false)
+ ::ApplicationRecord.transaction do
+ epic.move_between(before_epic, after_epic)
+ epic.save!(touch: false)
+ sync_work_items_relative_position!(before_epic, after_epic, epic)
+ end
+ end
+
+ def sync_work_items_relative_position!(before_epic, after_epic, epic)
+ return true unless sync_work_item_parent_links?(epic, before_epic, after_epic)
+
+ parent_link = epic.work_item.parent_link
+
+ parent_link.move_between(before_epic&.work_item&.parent_link, after_epic&.work_item&.parent_link)
+ return true if parent_link.save!(touch: false)
+
+ error = "Not able to sync child relative position: #{parent_link.errors.full_messages.to_sentence}"
+ raise SyncAsWorkItem::SyncAsWorkItemError, error
+ end
+
+ def sync_work_item_parent_links?(epic, before_epic, after_epic)
+ return false unless epic.parent.group.sync_epic_work_item_order_enabled? && epic.work_item&.parent_link.present?
+ return false if after_epic && after_epic.work_item&.parent_link.blank?
+ return false if before_epic && before_epic.work_item&.parent_link.blank?
+
+ true
end
end
end
diff --git a/ee/spec/requests/groups/epics/epic_links_controller_spec.rb b/ee/spec/requests/groups/epics/epic_links_controller_spec.rb
index 25b222e64191a..821ed26833979 100644
--- a/ee/spec/requests/groups/epics/epic_links_controller_spec.rb
+++ b/ee/spec/requests/groups/epics/epic_links_controller_spec.rb
@@ -274,10 +274,10 @@ def get_epics
context 'when move_before_id is not a sibling epic' do
let(:move_before_epic) { create(:epic, group: group) }
- it 'returns status 404' do
+ it 'returns status 422' do
subject
- expect(response).to have_gitlab_http_status(:not_found)
+ expect(response).to have_gitlab_http_status(:unprocessable_entity)
end
end
end
diff --git a/ee/spec/services/epics/epic_links/update_service_spec.rb b/ee/spec/services/epics/epic_links/update_service_spec.rb
index 744c971e3e04f..cdb131e41a8a1 100644
--- a/ee/spec/services/epics/epic_links/update_service_spec.rb
+++ b/ee/spec/services/epics/epic_links/update_service_spec.rb
@@ -3,44 +3,61 @@
require 'spec_helper'
RSpec.describe Epics::EpicLinks::UpdateService, feature_category: :portfolio_management do
- let(:user) { create(:user) }
- let(:group) { create(:group) }
- let(:parent_epic) { create(:epic, group: group) }
+ let_it_be(:guest) { create(:user) }
+ let_it_be(:group) { create(:group).tap { |g| g.add_guest(guest) } }
+ let_it_be(:parent_epic) { create(:epic, group: group) }
- let(:child_epic1) { create(:epic, group: group, parent: parent_epic, relative_position: 1) }
- let(:child_epic2) { create(:epic, group: group, parent: parent_epic, relative_position: 2) }
- let(:child_epic3) { create(:epic, group: group, parent: parent_epic, relative_position: 300) }
- let(:child_epic4) { create(:epic, group: group, parent: parent_epic, relative_position: 400) }
+ let_it_be_with_reload(:child_epic1) { create_child_epic(1) }
+ let_it_be_with_reload(:child_epic2) { create_child_epic(2) }
+ let_it_be_with_reload(:child_epic3) { create_child_epic(300) }
+ let_it_be_with_reload(:child_epic4) { create_child_epic(400) }
let(:epic_to_move) { child_epic3 }
let(:params) { {} }
+ let(:current_user) { guest }
- subject do
- described_class.new(epic_to_move, user, params).execute
+ subject(:reorder_child) do
+ described_class.new(epic_to_move, current_user, params).execute
end
def ordered_epics
Epic.where(parent_id: parent_epic.id).order('relative_position, id DESC')
end
- describe '#execute' do
- before do
- group.add_guest(user)
- end
+ def ordered_work_items
+ parent_epic.work_item.reload.work_item_children_by_relative_position
+ end
+
+ def create_child_epic(relative_position)
+ child = create(:epic, group: group, parent: parent_epic, relative_position: relative_position)
+ create(
+ :parent_link,
+ work_item: child.work_item,
+ work_item_parent: parent_epic.work_item,
+ relative_position: relative_position
+ )
+
+ child
+ end
+ describe '#execute' do
shared_examples 'updating timestamps' do
it 'does not update moved epic' do
updated_at = epic_to_move.updated_at
- subject
+ work_item_updated_at = epic_to_move.work_item.updated_at
+ reorder_child
expect(epic_to_move.reload.updated_at.change(usec: 0)).to eq(updated_at.change(usec: 0))
+ expect(epic_to_move.work_item.updated_at.change(usec: 0)).to eq(work_item_updated_at.change(usec: 0))
end
it 'does not update parent epic' do
updated_at = parent_epic.updated_at
- subject
+ work_item_updated_at = parent_epic.work_item.updated_at
+ reorder_child
expect(parent_epic.reload.updated_at.change(usec: 0)).to eq(updated_at.change(usec: 0))
+ expect(parent_epic.work_item.updated_at.change(usec: 0)).to eq(work_item_updated_at.change(usec: 0))
end
end
@@ -48,7 +65,7 @@ def ordered_epics
it 'returns an error' do
stub_licensed_features(epics: true, subepics: false)
- expect(subject).to eq(message: 'Epic not found for given params', status: :error, http_status: 404)
+ expect(reorder_child).to eq(message: 'Epic not found for given params', status: :error, http_status: 404)
end
end
@@ -58,12 +75,10 @@ def ordered_epics
end
context 'when user has insufficient permissions' do
- before do
- user.group_members.delete_all
- end
+ let(:current_user) { create(:user) }
it 'returns an error' do
- expect(subject).to eq(message: 'Epic not found for given params', status: :error, http_status: 404)
+ expect(reorder_child).to eq(message: 'Epic not found for given params', status: :error, http_status: 404)
end
end
@@ -71,8 +86,11 @@ def ordered_epics
let(:params) { { move_before_id: nil, move_after_id: nil } }
it 'does not change order of child epics' do
- expect(subject).to include(status: :success)
- expect(ordered_epics).to eq([child_epic1, child_epic2, child_epic3, child_epic4])
+ expect(reorder_child).to include(status: :success)
+ expect(ordered_epics).to match_array([child_epic1, child_epic2, child_epic3, child_epic4])
+ expect(ordered_work_items).to match_array([
+ child_epic1.work_item, child_epic2.work_item, child_epic3.work_item, child_epic4.work_item
+ ])
end
end
@@ -81,9 +99,12 @@ def ordered_epics
it_behaves_like 'updating timestamps'
- it 'reorders child epics' do
- expect(subject).to include(status: :success)
- expect(ordered_epics).to eq([child_epic3, child_epic1, child_epic2, child_epic4])
+ it 'reorders child epics and sync positions with work items' do
+ expect(reorder_child).to include(status: :success)
+ expect(ordered_epics).to match_array([child_epic3, child_epic1, child_epic2, child_epic4])
+ expect(ordered_work_items).to match_array(
+ [child_epic3.work_item, child_epic1.work_item, child_epic2.work_item, child_epic4.work_item]
+ )
end
end
@@ -92,9 +113,12 @@ def ordered_epics
it_behaves_like 'updating timestamps'
- it 'reorders child epics' do
- expect(subject).to include(status: :success)
- expect(ordered_epics).to eq([child_epic1, child_epic2, child_epic4, child_epic3])
+ it 'reorders child epics and sync positions with work items' do
+ expect(reorder_child).to include(status: :success)
+ expect(ordered_epics).to match_array([child_epic1, child_epic2, child_epic4, child_epic3])
+ expect(ordered_work_items).to match_array(
+ [child_epic1.work_item, child_epic2.work_item, child_epic4.work_item, child_epic3.work_item]
+ )
end
end
@@ -104,18 +128,26 @@ def ordered_epics
it_behaves_like 'updating timestamps'
it 'reorders child epics' do
- expect(subject).to include(status: :success)
- expect(ordered_epics).to eq([child_epic1, child_epic3, child_epic2, child_epic4])
+ expect(reorder_child).to include(status: :success)
+ expect(ordered_epics).to match_array([child_epic1, child_epic3, child_epic2, child_epic4])
+ expect(ordered_work_items).to match_array(
+ [child_epic1.work_item, child_epic3.work_item, child_epic2.work_item, child_epic4.work_item]
+ )
end
end
context 'when params are invalid' do
- let(:other_epic) { create(:epic, group: group) }
+ let_it_be(:other_epic) { create(:epic, group: group) }
shared_examples 'returns error' do
it 'does not change order of child epics and returns error' do
- expect(subject).to include(message: 'Epic not found for given params', status: :error, http_status: 404)
- expect(ordered_epics).to eq([child_epic1, child_epic2, child_epic3, child_epic4])
+ expect(reorder_child).to include(
+ message: 'Epic not found for given params', status: :error, http_status: 422
+ )
+ expect(ordered_epics).to match_array([child_epic1, child_epic2, child_epic3, child_epic4])
+ expect(ordered_work_items).to match_array(
+ [child_epic1.work_item, child_epic2.work_item, child_epic3.work_item, child_epic4.work_item]
+ )
end
end
@@ -131,6 +163,83 @@ def ordered_epics
it_behaves_like 'returns error'
end
end
+
+ context 'when syncing with work items fails' do
+ let(:params) { { move_before_id: child_epic4.id, move_after_id: nil } }
+
+ it 'does not change order of child epics and returns error' do
+ allow(epic_to_move.work_item.parent_link).to receive(:save!).and_return(false)
+
+ expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
+ instance_of(Epics::SyncAsWorkItem::SyncAsWorkItemError), { moving_epic_id: epic_to_move.id }
+ )
+ expect(reorder_child).to include(
+ message: "Couldn't reorder child due to an internal error.", status: :error, http_status: 422
+ )
+ expect(ordered_epics).to match_array([child_epic1, child_epic2, child_epic3, child_epic4])
+ expect(ordered_work_items).to match_array(
+ [child_epic1.work_item, child_epic2.work_item, child_epic3.work_item, child_epic4.work_item]
+ )
+ end
+ end
+
+ context 'when work item should not be synced' do
+ let(:params) { { move_before_id: child_epic1.id, move_after_id: child_epic2.id } }
+ let(:expected_work_items) do
+ [child_epic1.work_item, child_epic2.work_item, child_epic3.work_item, child_epic4.work_item]
+ end
+
+ shared_examples 'reordering without syncing relative positions' do
+ it 'only changes order of child epics and not the order of synced work items' do
+ expect(reorder_child).to include(status: :success)
+ expect(ordered_epics).to match_array([child_epic1, child_epic3, child_epic2, child_epic4])
+ expect(ordered_work_items).to match_array(expected_work_items)
+ end
+ end
+
+ context 'when sync_epic_to_work_item feature flag is disabled' do
+ before do
+ stub_feature_flags(sync_epic_to_work_item: false)
+ end
+
+ it_behaves_like 'reordering without syncing relative positions'
+ end
+
+ context 'when sync_epic_work_item_order feature flag is disabled' do
+ before do
+ stub_feature_flags(sync_epic_work_item_order: false)
+ end
+
+ it_behaves_like 'reordering without syncing relative positions'
+ end
+
+ context 'when moving child does not have a synced work item parent link' do
+ before do
+ WorkItems::ParentLink.where(work_item_id: epic_to_move.issue_id).delete_all
+ expected_work_items.delete_at(2)
+ end
+
+ it_behaves_like 'reordering without syncing relative positions'
+ end
+
+ context 'when move_before child does not have a synced work item parent link' do
+ before do
+ WorkItems::ParentLink.where(work_item_id: child_epic1.issue_id).delete_all
+ expected_work_items.delete_at(0)
+ end
+
+ it_behaves_like 'reordering without syncing relative positions'
+ end
+
+ context 'when move_after child does not have a synced work item parent link' do
+ before do
+ WorkItems::ParentLink.where(work_item_id: child_epic2.issue_id).delete_all
+ expected_work_items.delete_at(1)
+ end
+
+ it_behaves_like 'reordering without syncing relative positions'
+ end
+ end
end
end
end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index ad39104531418..839e633468dce 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -14998,6 +14998,9 @@ msgstr ""
msgid "Couldn't link epics. You must have at least the Guest role in the epic's group."
msgstr ""
+msgid "Couldn't reorder child due to an internal error."
+msgstr ""
+
msgid "Country / Region"
msgstr ""
--
GitLab