Merge branch...

Merge branch '421030-add-the-ability-to-query-descendant-milestones-via-rest-and-graphql-api' into 'master' 

Add the ability to query descendant milestones via REST API

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/138380



Merged-by: Aleksei Lipniagov <alipniagov@gitlab.com>
Approved-by: Ravi Kumar <rkumar@gitlab.com>
Approved-by: Aleksei Lipniagov <alipniagov@gitlab.com>
Reviewed-by: Aleksei Lipniagov <alipniagov@gitlab.com>
Co-authored-by: Ravi Kumar <rkumar@gitlab.com>
Co-authored-by: Brett Walker <bwalker@gitlab.com>

app/graphql/resolvers/group_milestones_resolver.rb

@@ -2,6 +2,8 @@
 
 module Resolvers
   class GroupMilestonesResolver < MilestonesResolver
+    include ::API::Concerns::Milestones::GroupProjectParams
+
     argument :include_ancestors, GraphQL::Types::Boolean,
              required: false,
              description: 'Include milestones from all parent groups.'
@@ -14,36 +16,7 @@ class GroupMilestonesResolver < MilestonesResolver
     private
 
     def parent_id_parameters(args)
-      include_ancestors = args[:include_ancestors].present?
-      include_descendants = args[:include_descendants].present?
-      return { group_ids: parent.id } unless include_ancestors || include_descendants
-
-      group_ids = if include_ancestors && include_descendants
-                    parent.self_and_hierarchy
-                  elsif include_ancestors
-                    parent.self_and_ancestors
-                  else
-                    parent.self_and_descendants
-                  end
-
-      project_ids = if include_descendants
-                      group_projects.with_issues_or_mrs_available_for_user(current_user)
-                    else
-                      nil
-                    end
-
-      {
-        group_ids: group_ids.public_or_visible_to_user(current_user).select(:id),
-        project_ids: project_ids
-      }
-    end
-
-    def group_projects
-      GroupProjectsFinder.new(
-        group: parent,
-        current_user: current_user,
-        options: { include_subgroups: true }
-      ).execute
+      group_finder_params(parent, args)
     end
 
     def preloads

app/graphql/resolvers/project_milestones_resolver.rb

@@ -2,6 +2,8 @@
 
 module Resolvers
   class ProjectMilestonesResolver < MilestonesResolver
+    include ::API::Concerns::Milestones::GroupProjectParams
+
     argument :include_ancestors, GraphQL::Types::Boolean,
              required: false,
              description: "Also return milestones in the project's parent group and its ancestors."
@@ -11,12 +13,7 @@ class ProjectMilestonesResolver < MilestonesResolver
     private
 
     def parent_id_parameters(args)
-      return { project_ids: parent.id } unless args[:include_ancestors].present? && parent.group.present?
-
-      {
-        group_ids: parent.group.self_and_ancestors.select(:id),
-        project_ids: parent.id
-      }
+      project_finder_params(parent, args)
     end
   end
 end

doc/api/group_milestones.md

@@ -35,7 +35,8 @@ Parameters:
 | `title`                     | string | no | Return only the milestones having the given `title` |
 | `search`                    | string | no | Return only milestones with a title or description matching the provided string |
 | `include_parent_milestones` | boolean | no | [Deprecated](https://gitlab.com/gitlab-org/gitlab/-/issues/433298) in GitLab 16.7. Use `include_ancestors` instead. |
-| `include_ancestors`         | boolean | no | Include milestones from all parent groups. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/196066) in GitLab 13.4. |
+| `include_ancestors`         | boolean | no | Include milestones for all parent groups. |
+| `include_descendants`       | boolean | no | Include milestones for group and its descendants. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/421030) in GitLab 16.7. |
 | `updated_before`            | datetime | no | Return only milestones updated before the given datetime. Expected in ISO 8601 format (`2019-03-15T08:00:00Z`). Introduced in GitLab 15.10 |
 | `updated_after`             | datetime | no | Return only milestones updated after the given datetime. Expected in ISO 8601 format (`2019-03-15T08:00:00Z`). Introduced in GitLab 15.10 |
 

lib/api/concerns/milestones/group_project_params.rb

+# frozen_string_literal: true
+
+# This DRYs up some methods used by both the GraphQL and REST milestone APIs
+module API
+  module Concerns
+    module Milestones
+      module GroupProjectParams
+        extend ActiveSupport::Concern
+
+        private
+
+        def project_finder_params(parent, params)
+          return { project_ids: parent.id } unless params[:include_ancestors].present? && parent.group.present?
+
+          {
+            group_ids: parent.group.self_and_ancestors.select(:id),
+            project_ids: parent.id
+          }
+        end
+
+        def group_finder_params(parent, params)
+          include_ancestors = params[:include_ancestors].present?
+          include_descendants = params[:include_descendants].present?
+          return { group_ids: parent.id } unless include_ancestors || include_descendants
+
+          group_ids = if include_ancestors && include_descendants
+                        parent.self_and_hierarchy
+                      elsif include_ancestors
+                        parent.self_and_ancestors
+                      else
+                        parent.self_and_descendants
+                      end
+
+          if include_descendants
+            project_ids = group_projects(parent).with_issues_or_mrs_available_for_user(current_user)
+          end
+
+          {
+            group_ids: group_ids.public_or_visible_to_user(current_user).select(:id),
+            project_ids: project_ids
+          }
+        end
+
+        def group_projects(parent)
+          GroupProjectsFinder.new(
+            group: parent,
+            current_user: current_user,
+            options: { include_subgroups: true }
+          ).execute
+        end
+      end
+    end
+  end
+end

lib/api/group_milestones.rb

@@ -19,6 +19,8 @@ class GroupMilestones < ::API::Base
       end
       params do
         use :list_params
+        optional :include_descendants, type: Grape::API::Boolean,
+          desc: 'Include milestones from all subgroups and subprojects'
       end
       get ":id/milestones" do
         list_milestones_for(user_group)

lib/api/milestone_responses.rb

@@ -6,6 +6,8 @@ module MilestoneResponses
 
     included do
       helpers do
+        include ::API::Concerns::Milestones::GroupProjectParams
+
         params :optional_params do
           optional :description, type: String, desc: 'The description of the milestone'
           optional :due_date, type: String, desc: 'The due date of the milestone. The ISO 8601 date format (%Y-%m-%d)'
@@ -90,12 +92,10 @@ def milestone_issuables_for(parent, type)
         end
 
         def parent_finder_params(parent)
-          include_parent = params[:include_ancestors].present?
-
           if parent.is_a?(Project)
-            { project_ids: parent.id, group_ids: (include_parent ? project_group_ids(parent) : nil) }
+            project_finder_params(parent, params)
           else
-            { group_ids: (include_parent ? group_and_ancestor_ids(parent) : parent.id) }
+            group_finder_params(parent, params)
           end
         end
 
@@ -116,21 +116,6 @@ def get_finder_and_entity(type)
             [MergeRequestsFinder, Entities::MergeRequestBasic]
           end
         end
-
-        def project_group_ids(parent)
-          group = parent.group
-          return unless group.present?
-
-          group.self_and_ancestors.select(:id)
-        end
-
-        def group_and_ancestor_ids(group)
-          return unless group.present?
-
-          group.self_and_ancestors
-            .public_or_visible_to_user(current_user)
-            .select(:id)
-        end
       end
     end
   end

spec/graphql/resolvers/group_milestones_resolver_spec.rb

@@ -102,76 +102,7 @@ def args(**arguments)
       end
     end
 
-    context 'when including descendant milestones in a public group' do
-      let_it_be(:group) { create(:group, :public) }
-
-      let(:args) { { include_descendants: true } }
-
-      it 'finds milestones only in accessible projects and groups' do
-        accessible_group = create(:group, :private, parent: group)
-        accessible_project = create(:project, group: accessible_group)
-        accessible_group.add_developer(current_user)
-        inaccessible_group = create(:group, :private, parent: group)
-        inaccessible_project = create(:project, :private, group: group)
-        milestone1 = create(:milestone, group: group)
-        milestone2 = create(:milestone, group: accessible_group)
-        milestone3 = create(:milestone, project: accessible_project)
-        create(:milestone, group: inaccessible_group)
-        create(:milestone, project: inaccessible_project)
-
-        expect(resolve_group_milestones(args: args)).to match_array([milestone1, milestone2, milestone3])
-      end
-    end
-
-    describe 'include_descendants and include_ancestors' do
-      let_it_be(:parent_group) { create(:group, :public) }
-      let_it_be(:group) { create(:group, :public, parent: parent_group) }
-      let_it_be(:accessible_group) { create(:group, :private, parent: group) }
-      let_it_be(:accessible_project) { create(:project, group: accessible_group) }
-      let_it_be(:inaccessible_group) { create(:group, :private, parent: group) }
-      let_it_be(:inaccessible_project) { create(:project, :private, group: group) }
-      let_it_be(:milestone1) { create(:milestone, group: group) }
-      let_it_be(:milestone2) { create(:milestone, group: accessible_group) }
-      let_it_be(:milestone3) { create(:milestone, project: accessible_project) }
-      let_it_be(:milestone4) { create(:milestone, group: inaccessible_group) }
-      let_it_be(:milestone5) { create(:milestone, project: inaccessible_project) }
-      let_it_be(:milestone6) { create(:milestone, group: parent_group) }
-
-      before do
-        accessible_group.add_developer(current_user)
-      end
-
-      context 'when including neither ancestor or descendant milestones in a public group' do
-        let(:args) { {} }
-
-        it 'finds milestones only in accessible projects and groups' do
-          expect(resolve_group_milestones(args: args)).to match_array([milestone1])
-        end
-      end
-
-      context 'when including descendant milestones in a public group' do
-        let(:args) { { include_descendants: true } }
-
-        it 'finds milestones only in accessible projects and groups' do
-          expect(resolve_group_milestones(args: args)).to match_array([milestone1, milestone2, milestone3])
-        end
-      end
-
-      context 'when including ancestor milestones in a public group' do
-        let(:args) { { include_ancestors: true } }
-
-        it 'finds milestones only in accessible projects and groups' do
-          expect(resolve_group_milestones(args: args)).to match_array([milestone1, milestone6])
-        end
-      end
-
-      context 'when including both ancestor or descendant milestones in a public group' do
-        let(:args) { { include_descendants: true, include_ancestors: true } }
-
-        it 'finds milestones only in accessible projects and groups' do
-          expect(resolve_group_milestones(args: args)).to match_array([milestone1, milestone2, milestone3, milestone6])
-        end
-      end
-    end
+    # testing for include_descendants and include_ancestors moved into
+    # `spec/requests/api/graphql/milestone_spec.rb`
   end
 end

spec/requests/api/graphql/milestone_spec.rb

@@ -151,4 +151,18 @@
       end
     end
   end
+
+  context 'for common GraphQL/REST' do
+    it_behaves_like 'group milestones including ancestors and descendants'
+
+    def query_group_milestone_ids(params)
+      query = graphql_query_for('group', { 'fullPath' => group.full_path },
+        query_graphql_field('milestones', params, query_graphql_path([:nodes], :id))
+      )
+
+      post_graphql(query, current_user: current_user)
+
+      graphql_data_at(:group, :milestones, :nodes).pluck('id').map { |gid| GlobalID.parse(gid).model_id.to_i }
+    end
+  end
 end

spec/requests/api/group_milestones_spec.rb

@@ -30,91 +30,103 @@
   it_behaves_like 'group and project milestones', "/groups/:id/milestones"
 
   describe 'GET /groups/:id/milestones' do
-    let_it_be(:ancestor_group) { create(:group, :private) }
-    let_it_be(:ancestor_group_milestone) { create(:milestone, group: ancestor_group, updated_at: 2.days.ago) }
+    context 'for REST only' do
+      let_it_be(:ancestor_group) { create(:group, :private) }
+      let_it_be(:ancestor_group_milestone) { create(:milestone, group: ancestor_group, updated_at: 2.days.ago) }
 
-    before_all do
-      group.update!(parent: ancestor_group)
-    end
+      before_all do
+        group.update!(parent: ancestor_group)
+      end
 
-    context 'when include_ancestors is true' do
-      let(:params) { { include_ancestors: true } }
+      context 'when include_ancestors is true' do
+        let(:params) { { include_ancestors: true } }
 
-      context 'when user has access to ancestor groups' do
-        let(:milestones) { [ancestor_group_milestone, milestone, closed_milestone] }
+        context 'when user has access to ancestor groups' do
+          let(:milestones) { [ancestor_group_milestone, milestone, closed_milestone] }
 
-        before do
-          ancestor_group.add_guest(user)
-          group.add_guest(user)
-        end
+          before do
+            ancestor_group.add_guest(user)
+            group.add_guest(user)
+          end
 
-        it_behaves_like 'listing all milestones'
+          it_behaves_like 'listing all milestones'
 
-        context 'when deprecated include_parent_milestones is true' do
-          let(:params) { { include_parent_milestones: true } }
+          context 'when deprecated include_parent_milestones is true' do
+            let(:params) { { include_parent_milestones: true } }
 
-          it_behaves_like 'listing all milestones'
-        end
+            it_behaves_like 'listing all milestones'
+          end
 
-        context 'when both include_parent_milestones and include_ancestors are specified' do
-          let(:params) { { include_ancestors: true, include_parent_milestones: true } }
+          context 'when both include_parent_milestones and include_ancestors are specified' do
+            let(:params) { { include_ancestors: true, include_parent_milestones: true } }
 
-          it 'returns 400' do
-            get api(route, user), params: params
+            it 'returns 400' do
+              get api(route, user), params: params
 
-            expect(response).to have_gitlab_http_status(:bad_request)
+              expect(response).to have_gitlab_http_status(:bad_request)
+            end
           end
-        end
 
-        context 'when iids param is present' do
-          let(:params) { { include_ancestors: true, iids: [milestone.iid] } }
+          context 'when iids param is present' do
+            let(:params) { { include_ancestors: true, iids: [milestone.iid] } }
 
-          it_behaves_like 'listing all milestones'
-        end
+            it_behaves_like 'listing all milestones'
+          end
 
-        context 'when updated_before param is present' do
-          let(:params) { { updated_before: 1.day.ago.iso8601, include_ancestors: true } }
+          context 'when updated_before param is present' do
+            let(:params) { { updated_before: 1.day.ago.iso8601, include_ancestors: true } }
 
-          it_behaves_like 'listing all milestones' do
-            let(:milestones) { [ancestor_group_milestone, milestone] }
+            it_behaves_like 'listing all milestones' do
+              let(:milestones) { [ancestor_group_milestone, milestone] }
+            end
+          end
+
+          context 'when updated_after param is present' do
+            let(:params) { { updated_after: 1.day.ago.iso8601, include_ancestors: true } }
+
+            it_behaves_like 'listing all milestones' do
+              let(:milestones) { [closed_milestone] }
+            end
           end
         end
 
-        context 'when updated_after param is present' do
-          let(:params) { { updated_after: 1.day.ago.iso8601, include_ancestors: true } }
+        context 'when user has no access to ancestor groups' do
+          let(:user) { create(:user) }
+
+          before do
+            group.add_guest(user)
+          end
 
           it_behaves_like 'listing all milestones' do
-            let(:milestones) { [closed_milestone] }
+            let(:milestones) { [milestone, closed_milestone] }
           end
         end
       end
 
-      context 'when user has no access to ancestor groups' do
-        let(:user) { create(:user) }
-
-        before do
-          group.add_guest(user)
-        end
+      context 'when updated_before param is present' do
+        let(:params) { { updated_before: 1.day.ago.iso8601 } }
 
         it_behaves_like 'listing all milestones' do
-          let(:milestones) { [milestone, closed_milestone] }
+          let(:milestones) { [milestone] }
         end
       end
-    end
 
-    context 'when updated_before param is present' do
-      let(:params) { { updated_before: 1.day.ago.iso8601 } }
+      context 'when updated_after param is present' do
+        let(:params) { { updated_after: 1.day.ago.iso8601 } }
 
-      it_behaves_like 'listing all milestones' do
-        let(:milestones) { [milestone] }
+        it_behaves_like 'listing all milestones' do
+          let(:milestones) { [closed_milestone] }
+        end
       end
     end
 
-    context 'when updated_after param is present' do
-      let(:params) { { updated_after: 1.day.ago.iso8601 } }
+    context 'for common GraphQL/REST' do
+      it_behaves_like 'group milestones including ancestors and descendants'
+
+      def query_group_milestone_ids(params)
+        get api(route, current_user), params: params
 
-      it_behaves_like 'listing all milestones' do
-        let(:milestones) { [closed_milestone] }
+        json_response.pluck('id')
       end
     end
   end

spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb

+# frozen_string_literal: true
+
+# Examples for both GraphQL and REST APIs
+RSpec.shared_examples 'group milestones including ancestors and descendants' do
+  context 'for group milestones' do
+    let_it_be(:current_user) { create(:user) }
+
+    context 'when including descendant milestones in a public group' do
+      let_it_be(:group) { create(:group, :public) }
+
+      let(:params) { { include_descendants: true } }
+
+      it 'finds milestones only in accessible projects and groups' do
+        accessible_group = create(:group, :private, parent: group)
+        accessible_project = create(:project, group: accessible_group)
+        accessible_group.add_developer(current_user)
+        inaccessible_group = create(:group, :private, parent: group)
+        inaccessible_project = create(:project, :private, group: group)
+        milestone1 = create(:milestone, group: group)
+        milestone2 = create(:milestone, group: accessible_group)
+        milestone3 = create(:milestone, project: accessible_project)
+        create(:milestone, group: inaccessible_group)
+        create(:milestone, project: inaccessible_project)
+
+        milestone_ids = query_group_milestone_ids(params)
+
+        expect(milestone_ids).to match_array([milestone1, milestone2, milestone3].pluck(:id))
+      end
+    end
+
+    describe 'include_descendants and include_ancestors' do
+      let_it_be(:parent_group) { create(:group, :public) }
+      let_it_be(:group) { create(:group, :public, parent: parent_group) }
+      let_it_be(:accessible_group) { create(:group, :private, parent: group) }
+      let_it_be(:accessible_project) { create(:project, group: accessible_group) }
+      let_it_be(:inaccessible_group) { create(:group, :private, parent: group) }
+      let_it_be(:inaccessible_project) { create(:project, :private, group: group) }
+      let_it_be(:milestone1) { create(:milestone, group: group) }
+      let_it_be(:milestone2) { create(:milestone, group: accessible_group) }
+      let_it_be(:milestone3) { create(:milestone, project: accessible_project) }
+      let_it_be(:milestone4) { create(:milestone, group: inaccessible_group) }
+      let_it_be(:milestone5) { create(:milestone, project: inaccessible_project) }
+      let_it_be(:milestone6) { create(:milestone, group: parent_group) }
+
+      before_all do
+        accessible_group.add_developer(current_user)
+      end
+
+      context 'when including neither ancestor nor descendant milestones in a public group' do
+        let(:params) { {} }
+
+        it 'finds milestones only in accessible projects and groups' do
+          expect(query_group_milestone_ids(params)).to match_array([milestone1.id])
+        end
+      end
+
+      context 'when including descendant milestones in a public group' do
+        let(:params) { { include_descendants: true } }
+
+        it 'finds milestones only in accessible projects and groups' do
+          expect(query_group_milestone_ids(params)).to match_array([milestone1, milestone2, milestone3].pluck(:id))
+        end
+      end
+
+      context 'when including ancestor milestones in a public group' do
+        let(:params) { { include_ancestors: true } }
+
+        it 'finds milestones only in accessible projects and groups' do
+          expect(query_group_milestone_ids(params)).to match_array([milestone1, milestone6].pluck(:id))
+        end
+      end
+
+      context 'when including both ancestor and descendant milestones in a public group' do
+        let(:params) { { include_descendants: true, include_ancestors: true } }
+
+        it 'finds milestones only in accessible projects and groups' do
+          expect(query_group_milestone_ids(params))
+            .to match_array([milestone1, milestone2, milestone3, milestone6].pluck(:id))
+        end
+      end
+    end
+  end
+end