Merge branch '340578-epic-order-structure' into 'master'

Sort epic ancestors in hierarchical order in graphQL endpoint

See merge request gitlab-org/gitlab!72811

.rubocop_manual_todo.yml

@@ -32,7 +32,6 @@ Rails/SaveBang:
     - 'ee/spec/models/approval_project_rule_spec.rb'
     - 'ee/spec/models/burndown_spec.rb'
     - 'ee/spec/models/elasticsearch_indexed_namespace_spec.rb'
-    - 'ee/spec/models/epic_spec.rb'
     - 'ee/spec/models/gitlab_subscription_spec.rb'
     - 'ee/spec/models/issue_spec.rb'
     - 'ee/spec/models/label_note_spec.rb'

ee/app/finders/epics_finder.rb

@@ -27,6 +27,7 @@
 #   include_descendant_groups: boolean
 #   starts_with_iid: string (containing a number)
 #   confidential: boolean
+#   hierarchy_order: :desc or :acs, default :acs when searched by child_id
 
 class EpicsFinder < IssuableFinder
   include TimeFrameFilter
@@ -198,8 +199,10 @@ class EpicsFinder < IssuableFinder
   def by_child(items)
     return items unless child_id?
 
-    ancestor_ids = Epic.find(params[:child_id]).ancestors.reselect(:id)
-    items.where(id: ancestor_ids)
+    hierarchy_order = params[:hierarchy_order] || :asc
+
+    ancestors = Epic.find(params[:child_id]).ancestors(hierarchy_order: hierarchy_order)
+    ancestors.where(id: items.select(:id))
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
@@ -272,4 +275,11 @@ class EpicsFinder < IssuableFinder
   def feature_flag_scope
     params.group
   end
+
+  override :sort
+  def sort(items)
+    return items if params[:hierarchy_order]
+
+    super
+  end
 end

ee/app/graphql/resolvers/epic_ancestors_resolver.rb

@@ -9,12 +9,18 @@ module Resolvers
              description: 'Include epics from ancestor groups.',
              default_value: true
 
+    def resolve_with_lookahead(**args)
+      items = super
+
+      offset_pagination(items)
+    end
+
     private
 
     def relative_param
       return {} unless parent
 
-      { child_id: parent.id }
+      { child_id: parent.id, hierarchy_order: :desc }
     end
   end
 end

ee/app/models/ee/epic.rb

@@ -425,10 +425,10 @@ module EE
       end
     end
 
-    def ancestors
+    def ancestors(hierarchy_order: :asc)
       return self.class.none unless parent_id
 
-      hierarchy.ancestors(hierarchy_order: :asc)
+      hierarchy.ancestors(hierarchy_order: hierarchy_order)
     end
 
     def max_hierarchy_depth_achieved?

ee/spec/finders/epics_finder_spec.rb

@@ -9,8 +9,9 @@ RSpec.describe EpicsFinder do
   let_it_be(:another_group) { create(:group) }
   let_it_be(:reference_time) { Time.parse('2020-09-15 01:00') } # Arbitrary time used for time/date range filters
   let_it_be(:epic1) { create(:epic, :opened, group: group, title: 'This is awesome epic', created_at: 1.week.before(reference_time), end_date: 10.days.before(reference_time)) }
-  let_it_be(:epic2) { create(:epic, :opened, group: group, created_at: 4.days.before(reference_time), author: user, start_date: 2.days.before(reference_time), end_date: 3.days.since(reference_time), parent: epic1) }
-  let_it_be(:epic3) { create(:epic, :closed, group: group, description: 'not so awesome', start_date: 5.days.before(reference_time), end_date: 3.days.before(reference_time), parent: epic2) }
+  let_it_be(:epic3, reload: true) { create(:epic, :closed, group: group, description: 'not so awesome', start_date: 5.days.before(reference_time), end_date: 3.days.before(reference_time)) }
+  let_it_be(:epic2, reload: true) { create(:epic, :opened, group: group, created_at: 4.days.before(reference_time), author: user, start_date: 2.days.before(reference_time), end_date: 3.days.since(reference_time)) }
+  let_it_be(:epic5) { create(:epic, group: group, start_date: 6.days.before(reference_time), end_date: 6.days.before(reference_time), parent: epic3) }
   let_it_be(:epic4) { create(:epic, :closed, group: another_group) }
 
   describe '#execute' do
@@ -55,7 +56,7 @@ RSpec.describe EpicsFinder do
         end
 
         it 'returns all epics that belong to the given group' do
-          expect(epics).to contain_exactly(epic1, epic2, epic3)
+          expect(epics).to contain_exactly(epic1, epic2, epic3, epic5)
         end
 
         it 'does not execute more than 5 SQL queries' do
@@ -64,11 +65,11 @@ RSpec.describe EpicsFinder do
 
         context 'sorting' do
           it 'sorts correctly when supported sorting param provided' do
-            expect(epics(sort: :start_date_asc)).to eq([epic3, epic2, epic1])
+            expect(epics(sort: :start_date_asc)).to eq([epic5, epic3, epic2, epic1])
           end
 
           it 'sorts by id when not supported sorting param provided' do
-            expect(epics(sort: :not_supported_param)).to eq([epic3, epic2, epic1])
+            expect(epics(sort: :not_supported_param)).to eq([epic5, epic2, epic3, epic1])
           end
         end
 
@@ -78,7 +79,7 @@ RSpec.describe EpicsFinder do
           end
 
           it 'returns all epics created after the given date' do
-            expect(epics(created_after: 2.days.before(reference_time))).to contain_exactly(epic3)
+            expect(epics(created_after: 2.days.before(reference_time))).to contain_exactly(epic3, epic5)
           end
 
           it 'returns all epics created within the given interval' do
@@ -146,7 +147,7 @@ RSpec.describe EpicsFinder do
               end
 
               it 'does not add any filter' do
-                expect(epics(or: { author_username: [epic2.author.username, epic3.author.username] })).to contain_exactly(epic1, epic2, epic3)
+                expect(epics(or: { author_username: [epic2.author.username, epic3.author.username] })).to contain_exactly(epic1, epic2, epic3, epic5)
               end
             end
           end
@@ -195,7 +196,7 @@ RSpec.describe EpicsFinder do
           end
 
           it 'returns all epics that belong to the given group and its subgroups' do
-            expect(epics).to contain_exactly(epic1, epic2, epic3, subgroup_epic, subgroup2_epic)
+            expect(epics).to contain_exactly(epic1, epic2, epic3, subgroup_epic, subgroup2_epic, epic5)
           end
 
           describe 'hierarchy params' do
@@ -223,7 +224,7 @@ RSpec.describe EpicsFinder do
               subject { finder.execute }
 
               it 'gets only epics from the project ancestor groups' do
-                is_expected.to contain_exactly(epic1, epic2, epic3, subgroup3_epic)
+                is_expected.to contain_exactly(epic1, epic2, epic3, subgroup3_epic, epic5)
               end
             end
 
@@ -237,7 +238,7 @@ RSpec.describe EpicsFinder do
               context 'and include_ancestor_groups is true' do
                 let(:finder_params) { { include_descendant_groups: false, include_ancestor_groups: true } }
 
-                it { is_expected.to contain_exactly(subgroup_epic, epic1, epic2, epic3) }
+                it { is_expected.to contain_exactly(subgroup_epic, epic1, epic2, epic3, epic5) }
 
                 context "when user does not have permission to view ancestor groups" do
                   let(:finder_params) { { group_id: subgroup.id, include_descendant_groups: false, include_ancestor_groups: true } }
@@ -259,7 +260,7 @@ RSpec.describe EpicsFinder do
               context 'and include_ancestor_groups is true' do
                 let(:finder_params) { { include_ancestor_groups: true } }
 
-                it { is_expected.to contain_exactly(subgroup_epic, subgroup2_epic, epic1, epic2, epic3) }
+                it { is_expected.to contain_exactly(subgroup_epic, subgroup2_epic, epic1, epic2, epic3, epic5) }
 
                 context "when user does not have permission to view ancestor groups" do
                   let(:finder_params) { { group_id: subgroup.id, include_ancestor_groups: true } }
@@ -344,6 +345,11 @@ RSpec.describe EpicsFinder do
         end
 
         context 'by parent' do
+          before do
+            epic3.update!(parent_id: epic2.id)
+            epic2.update!(parent_id: epic1.id)
+          end
+
           it 'returns direct children of the parent' do
             params = { parent_id: epic1.id }
 
@@ -352,10 +358,21 @@ RSpec.describe EpicsFinder do
         end
 
         context 'by child' do
-          it 'returns ancestors of the child epic' do
-            params = { child_id: epic3.id }
+          before do
+            epic3.update!(parent_id: epic2.id)
+            epic2.update!(parent_id: epic1.id)
+          end
 
-            expect(epics(params)).to contain_exactly(epic1, epic2)
+          it 'returns ancestors of the child epic ordered from the bottom' do
+            params = { child_id: epic5.id, hierarchy_order: :asc }
+
+            expect(epics(params)).to eq([epic3, epic2, epic1])
+          end
+
+          it 'returns ancestors of the child epic ordered from the top if requested' do
+            params = { child_id: epic5.id, hierarchy_order: :desc }
+
+            expect(epics(params)).to eq([epic1, epic2, epic3])
           end
         end
 
@@ -741,11 +758,11 @@ RSpec.describe EpicsFinder do
           let_it_be(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
 
           it 'returns all epics if no negated labels are present' do
-            expect(epics).to contain_exactly(negated_epic, negated_epic2, epic1, epic2, epic3)
+            expect(epics).to contain_exactly(negated_epic, negated_epic2, epic1, epic2, epic3, epic5)
           end
 
           it 'returns all epics without negated label' do
-            expect(epics(params)).to contain_exactly(epic1, epic2, epic3)
+            expect(epics(params)).to contain_exactly(epic1, epic2, epic3, epic5)
           end
         end
 
@@ -755,11 +772,11 @@ RSpec.describe EpicsFinder do
           let_it_be(:params) { { not: { author_id: author.id } } }
 
           it 'returns all epics if no negated author is present' do
-            expect(epics).to contain_exactly(authored_epic, epic1, epic2, epic3)
+            expect(epics).to contain_exactly(authored_epic, epic1, epic2, epic3, epic5)
           end
 
           it 'returns all epics without given author' do
-            expect(epics(params)).to contain_exactly(epic1, epic2, epic3)
+            expect(epics(params)).to contain_exactly(epic1, epic2, epic3, epic5)
           end
         end
 
@@ -768,7 +785,7 @@ RSpec.describe EpicsFinder do
           let_it_be(:params) { { not: { my_reaction_emoji: awarded_emoji.name } } }
 
           it 'returns all epics without given emoji name' do
-            expect(epics(params)).to contain_exactly(epic1, epic2)
+            expect(epics(params)).to contain_exactly(epic1, epic2, epic5)
           end
         end
       end
@@ -820,7 +837,7 @@ RSpec.describe EpicsFinder do
     it 'returns correct counts' do
       results = described_class.new(search_user, group_id: group.id).count_by_state
 
-      expect(results).to eq('opened' => 2, 'closed' => 1, 'all' => 3)
+      expect(results).to eq('opened' => 3, 'closed' => 1, 'all' => 4)
     end
 
     it 'returns -1 if the query times out' do

ee/spec/graphql/resolvers/epic_ancestors_resolver_spec.rb

@@ -54,8 +54,8 @@ RSpec.describe Resolvers::EpicAncestorsResolver do
           sub_group.add_developer(current_user)
         end
 
-        it 'returns all ancestors' do
-          expect(resolve_ancestors(epic4, args)).to contain_exactly(epic1, epic2, epic3)
+        it 'returns all ancestors in the correct order' do
+          expect(resolve_ancestors(epic4, args)).to eq([epic1, epic2, epic3])
         end
 
         it 'does not return parent group epics when include_ancestor_groups is false' do

ee/spec/models/epic_spec.rb

@@ -361,13 +361,22 @@ RSpec.describe Epic do
   end
 
   context 'hierarchy' do
-    let(:epic1) { create(:epic, group: group) }
-    let(:epic2) { create(:epic, group: group, parent: epic1) }
-    let(:epic3) { create(:epic, group: group, parent: epic2) }
+    let_it_be(:epic2, reload: true) { create(:epic, group: group) }
+    let_it_be(:epic3) { create(:epic, group: group, parent: epic2) }
+    let_it_be(:epic4) { create(:epic, group: group, parent: epic3) }
+    let_it_be(:epic1) { create(:epic, group: group) }
+
+    before do
+      epic2.update!(parent_id: epic1.id)
+    end
 
     describe '#ancestors' do
-      it 'returns all ancestors for an epic' do
-        expect(epic3.ancestors).to eq [epic2, epic1]
+      it 'returns all ancestors for an epic ordered correctly' do
+        expect(epic4.ancestors).to eq([epic3, epic2, epic1])
+      end
+
+      it 'returns all ancestors for an epic ordered correctly with the hierarchy_order param' do
+        expect(epic4.ancestors(hierarchy_order: :desc)).to eq([epic1, epic2, epic3])
       end
 
       it 'returns an empty array if an epic does not have any parent' do
@@ -377,11 +386,11 @@ RSpec.describe Epic do
 
     describe '#descendants' do
       it 'returns all descendants for an epic' do
-        expect(epic1.descendants).to match_array([epic2, epic3])
+        expect(epic1.descendants).to match_array([epic2, epic3, epic4])
       end
 
       it 'returns an empty array if an epic does not have any descendants' do
-        expect(epic3.descendants).to be_empty
+        expect(epic4.descendants).to be_empty
       end
     end
   end
@@ -705,8 +714,7 @@ RSpec.describe Epic do
     end
 
     before do
-      epic.description = ref_text
-      epic.save
+      epic.update!(description: ref_text)
     end
 
     it 'creates new system notes for cross references' do

ee/spec/requests/api/graphql/epics/epic_resolver_spec.rb

@@ -106,6 +106,21 @@ RSpec.describe 'getting epics information' do
     end
   end
 
+  context 'query for epics with ancestors' do
+    let_it_be(:parent_epic) { create(:epic, group: group) }
+    let_it_be(:epic) { create(:epic, group: group, parent: parent_epic) }
+
+    it 'returns the ancestors' do
+      query_epic_with_ancestors(epic.iid)
+
+      ancestors = graphql_data['group']['epic']['ancestors']['nodes']
+
+      expect(ancestors.count).to eq(1)
+      expect(ancestors.first['id']).to eq(parent_epic.to_global_id.to_s)
+      expect(graphql_errors).to be_nil
+    end
+  end
+
   describe 'N+1 query checks' do
     let_it_be(:epic_a) { create(:epic, group: group) }
     let_it_be(:epic_b) { create(:epic, group: group) }
@@ -157,6 +172,24 @@ RSpec.describe 'getting epics information' do
     )
   end
 
+  def query_epic_with_ancestors(epic_iid)
+    epics_field = <<~NODE
+      epic(iid: #{epic_iid}) {
+        id
+        ancestors {
+          nodes {
+            id
+          }
+        }
+      }
+    NODE
+
+    post_graphql(
+      graphql_query_for('group', { 'fullPath' => group.full_path }, epics_field),
+      current_user: user
+    )
+  end
+
   def epics_query(group, field, value)
     epics_query_by_hash(group, field => value)
   end