From 7f363d2e204776bfa310ed3b07e76e7de9eedc12 Mon Sep 17 00:00:00 2001
From: Brett Walker <bwalker@gitlab.com>
Date: Tue, 12 Dec 2023 17:12:30 +0000
Subject: [PATCH] Add support for include_descendants
for group milestone REST API query
Changelog: added
---
.../resolvers/group_milestones_resolver.rb | 33 +----
.../resolvers/project_milestones_resolver.rb | 9 +-
doc/api/group_milestones.md | 3 +-
.../milestones/group_project_params.rb | 54 +++++++++
lib/api/group_milestones.rb | 2 +
lib/api/milestone_responses.rb | 23 +---
.../group_milestones_resolver_spec.rb | 73 +----------
spec/requests/api/graphql/milestone_spec.rb | 14 +++
spec/requests/api/group_milestones_spec.rb | 114 ++++++++++--------
.../milestones_shared_examples.rb | 83 +++++++++++++
10 files changed, 230 insertions(+), 178 deletions(-)
create mode 100644 lib/api/concerns/milestones/group_project_params.rb
create mode 100644 spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb
diff --git a/app/graphql/resolvers/group_milestones_resolver.rb b/app/graphql/resolvers/group_milestones_resolver.rb
index 9242be7f684a4..d9a664b6ec2c4 100644
--- a/app/graphql/resolvers/group_milestones_resolver.rb
+++ b/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
diff --git a/app/graphql/resolvers/project_milestones_resolver.rb b/app/graphql/resolvers/project_milestones_resolver.rb
index cb4e9a5cdf735..0a078836a6a77 100644
--- a/app/graphql/resolvers/project_milestones_resolver.rb
+++ b/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
diff --git a/doc/api/group_milestones.md b/doc/api/group_milestones.md
index 2552626f6c623..57994e069a57a 100644
--- a/doc/api/group_milestones.md
+++ b/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 |
diff --git a/lib/api/concerns/milestones/group_project_params.rb b/lib/api/concerns/milestones/group_project_params.rb
new file mode 100644
index 0000000000000..72d07d7dcdb29
--- /dev/null
+++ b/lib/api/concerns/milestones/group_project_params.rb
@@ -0,0 +1,54 @@
+# 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
diff --git a/lib/api/group_milestones.rb b/lib/api/group_milestones.rb
index 0096e466bef9d..55c5ddfe5570f 100644
--- a/lib/api/group_milestones.rb
+++ b/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)
diff --git a/lib/api/milestone_responses.rb b/lib/api/milestone_responses.rb
index 85493dfc19809..3ef30a5fcd38c 100644
--- a/lib/api/milestone_responses.rb
+++ b/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
diff --git a/spec/graphql/resolvers/group_milestones_resolver_spec.rb b/spec/graphql/resolvers/group_milestones_resolver_spec.rb
index b9b8ef1870beb..e9caf91ecb702 100644
--- a/spec/graphql/resolvers/group_milestones_resolver_spec.rb
+++ b/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
diff --git a/spec/requests/api/graphql/milestone_spec.rb b/spec/requests/api/graphql/milestone_spec.rb
index 2cea9fd0408dd..0dc2eabc3e1d2 100644
--- a/spec/requests/api/graphql/milestone_spec.rb
+++ b/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
diff --git a/spec/requests/api/group_milestones_spec.rb b/spec/requests/api/group_milestones_spec.rb
index 2b17ce1d40f86..82a4311f7d0a7 100644
--- a/spec/requests/api/group_milestones_spec.rb
+++ b/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
diff --git a/spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb b/spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb
new file mode 100644
index 0000000000000..8e147f43091af
--- /dev/null
+++ b/spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb
@@ -0,0 +1,83 @@
+# 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
--
GitLab