From a4fbf39eca4518598e893f6f1b81b8b69927c6f9 Mon Sep 17 00:00:00 2001 From: Alexandru Croitor Date: Fri, 19 Apr 2019 10:55:36 +0300 Subject: [PATCH] Move issue details to from IssueBasic to Issue entity Cleanup IssueBasic entity to keep it basic and move extra attributes to Issue entity which contains more details --- lib/api/entities.rb | 27 +++++++++++++------ lib/api/helpers/issues_helpers.rb | 1 + lib/api/issues.rb | 12 ++++----- .../api/schemas/public_api/v4/issue.json | 6 ++--- .../api/issues/get_project_issues_spec.rb | 4 +-- 5 files changed, 30 insertions(+), 20 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index a57c7e9f851..1f10dc6c0fc 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -542,13 +542,9 @@ module API class IssueBasic < ProjectEntity expose :closed_at expose :closed_by, using: Entities::UserBasic - expose :labels do |issue, options| + expose :labels do |issue| # Avoids an N+1 query since labels are preloaded - if options[:with_labels_data] - ::API::Entities::LabelBasic.represent(issue.labels.sort_by(&:title)) - else - issue.labels.map(&:title).sort - end + issue.labels.map(&:title).sort end expose :milestone, using: Entities::Milestone expose :assignees, :author, using: Entities::UserBasic @@ -564,8 +560,6 @@ module API expose :due_date expose :confidential expose :discussion_locked - expose(:has_tasks) {|issue, _| !issue.task_list_items.empty? } - expose :task_status, if: -> (issue, _) { !issue.task_list_items.empty? } expose :web_url do |issue| Gitlab::UrlBuilder.build(issue) @@ -579,6 +573,23 @@ module API class Issue < IssueBasic include ::API::Helpers::RelatedResourcesHelpers + expose :labels do |issue, options| + # Avoids an N+1 query since labels are preloaded + if options[:with_labels_data] + ::API::Entities::LabelBasic.represent(issue.labels.sort_by(&:title)) + else + issue.labels.map(&:title).sort + end + end + + expose(:has_tasks) do |issue, _| + !issue.task_list_items.empty? + end + + expose :task_status, if: -> (issue, _) do + !issue.task_list_items.empty? + end + expose :_links do expose :self do |issue| expose_url(api_v4_project_issue_path(id: issue.project_id, issue_iid: issue.iid)) diff --git a/lib/api/helpers/issues_helpers.rb b/lib/api/helpers/issues_helpers.rb index 00aaf5243b7..12bbc598532 100644 --- a/lib/api/helpers/issues_helpers.rb +++ b/lib/api/helpers/issues_helpers.rb @@ -37,6 +37,7 @@ module API issues = finder.execute.with_api_entity_associations order_by = declared_params[:sort].present? && %w(asc desc).include?(declared_params[:sort].downcase) issues = issues.reorder(order_options_with_tie_breaker) if order_by + issues # rubocop: enable CodeReuse/ActiveRecord end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 67da1c46480..6977657e356 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -82,7 +82,7 @@ module API resource :issues do desc "Get currently authenticated user's issues" do - success Entities::IssueBasic + success Entities::Issue end params do use :issues_params @@ -94,7 +94,7 @@ module API issues = paginate(find_issues) options = { - with: Entities::IssueBasic, + with: Entities::Issue, with_labels_data: declared_params[:with_labels_data], current_user: current_user, issuable_metadata: issuable_meta_data(issues, 'Issue') @@ -109,7 +109,7 @@ module API end resource :groups, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do desc 'Get a list of group issues' do - success Entities::IssueBasic + success Entities::Issue end params do use :issues_params @@ -120,7 +120,7 @@ module API issues = paginate(find_issues(group_id: group.id, include_subgroups: true)) options = { - with: Entities::IssueBasic, + with: Entities::Issue, with_labels_data: declared_params[:with_labels_data], current_user: current_user, issuable_metadata: issuable_meta_data(issues, 'Issue') @@ -149,7 +149,7 @@ module API include TimeTrackingEndpoints desc 'Get a list of project issues' do - success Entities::IssueBasic + success Entities::Issue end params do use :issues_params @@ -160,7 +160,7 @@ module API issues = paginate(find_issues(project_id: project.id)) options = { - with: Entities::IssueBasic, + with: Entities::Issue, with_labels_data: declared_params[:with_labels_data], current_user: current_user, project: user_project, diff --git a/spec/fixtures/api/schemas/public_api/v4/issue.json b/spec/fixtures/api/schemas/public_api/v4/issue.json index 455941f05da..147f53239e0 100644 --- a/spec/fixtures/api/schemas/public_api/v4/issue.json +++ b/spec/fixtures/api/schemas/public_api/v4/issue.json @@ -14,7 +14,7 @@ "labels": { "type": "array", "items": { - "$ref": "../../entities/label.json" + "type": "string" } }, "milestone": { @@ -79,8 +79,6 @@ "due_date": { "type": ["date", "null"] }, "confidential": { "type": "boolean" }, "web_url": { "type": "uri" }, - "has_tasks": {"type": "boolean"}, - "task_status": {"type": "string"}, "time_stats": { "time_estimate": { "type": "integer" }, "total_time_spent": { "type": "integer" }, @@ -93,6 +91,6 @@ "state", "created_at", "updated_at", "labels", "milestone", "assignees", "author", "user_notes_count", "upvotes", "downvotes", "due_date", "confidential", - "has_tasks", "web_url" + "web_url" ] } diff --git a/spec/requests/api/issues/get_project_issues_spec.rb b/spec/requests/api/issues/get_project_issues_spec.rb index 8771889f7a4..afd5bd2257f 100644 --- a/spec/requests/api/issues/get_project_issues_spec.rb +++ b/spec/requests/api/issues/get_project_issues_spec.rb @@ -160,12 +160,12 @@ describe API::Issues do it 'avoids N+1 queries' do get api("/projects/#{project.id}/issues", user) + create_list(:issue, 3, project: project) + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do get api("/projects/#{project.id}/issues", user) end.count - create_list(:issue, 3, project: project) - expect do get api("/projects/#{project.id}/issues", user) end.not_to exceed_all_query_limit(control_count)