Merge branch '28453-add-time-estimate-time-spent-to-api-issue-output' into 'master'

Add time stats to Issue and Merge Request API

Closes #28453

See merge request !13335

9 files changed, 207 insertions, 17 deletions

diff --git a/changelogs/unreleased/28453-add-time-estimate-time-spent-to-api-issue-output.yml b/changelogs/unreleased/28453-add-time-estimate-time-spent-to-api-issue-output.yml
new file mode 100644
index 00000000000..129cf505a3f
--- /dev/null
+++ b/changelogs/unreleased/28453-add-time-estimate-time-spent-to-api-issue-output.yml
@@ -0,0 +1,4 @@
+---
+title: Add time stats to Issue and Merge Request API
+merge_request: 13335
+author: @travismiller
diff --git a/doc/api/issues.md b/doc/api/issues.md
index f30ed08d0fa..14635114a31 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -101,6 +101,12 @@ Example response:
       "user_notes_count": 1,
       "due_date": "2016-07-22",
       "web_url": "http://example.com/example/example/issues/6",
+      "time_stats": {
+         "time_estimate": 0,
+         "total_time_spent": 0,
+         "human_time_estimate": null,
+         "human_total_time_spent": null
+      },
       "confidential": false
    }
 ]
@@ -198,6 +204,12 @@ Example response:
       "user_notes_count": 1,
       "due_date": null,
       "web_url": "http://example.com/example/example/issues/1",
+      "time_stats": {
+         "time_estimate": 0,
+         "total_time_spent": 0,
+         "human_time_estimate": null,
+         "human_total_time_spent": null
+      },
       "confidential": false
    }
 ]
@@ -296,6 +308,12 @@ Example response:
       "user_notes_count": 1,
       "due_date": "2016-07-22",
       "web_url": "http://example.com/example/example/issues/1",
+      "time_stats": {
+         "time_estimate": 0,
+         "total_time_spent": 0,
+         "human_time_estimate": null,
+         "human_total_time_spent": null
+      },
       "confidential": false
    }
 ]
@@ -372,6 +390,12 @@ Example response:
    "user_notes_count": 1,
    "due_date": null,
    "web_url": "http://example.com/example/example/issues/1",
+   "time_stats": {
+      "time_estimate": 0,
+      "total_time_spent": 0,
+      "human_time_estimate": null,
+      "human_total_time_spent": null
+   },
    "confidential": false,
    "_links": {
       "self": "http://example.com/api/v4/projects/1/issues/2",
@@ -440,6 +464,12 @@ Example response:
    "user_notes_count": 0,
    "due_date": null,
    "web_url": "http://example.com/example/example/issues/14",
+   "time_stats": {
+      "time_estimate": 0,
+      "total_time_spent": 0,
+      "human_time_estimate": null,
+      "human_total_time_spent": null
+   },
    "confidential": false,
    "_links": {
       "self": "http://example.com/api/v4/projects/1/issues/2",
@@ -509,6 +539,12 @@ Example response:
    "user_notes_count": 0,
    "due_date": "2016-07-22",
    "web_url": "http://example.com/example/example/issues/15",
+   "time_stats": {
+      "time_estimate": 0,
+      "total_time_spent": 0,
+      "human_time_estimate": null,
+      "human_total_time_spent": null
+   },
    "confidential": false,
    "_links": {
       "self": "http://example.com/api/v4/projects/1/issues/2",
@@ -601,6 +637,12 @@ Example response:
   },
   "due_date": null,
   "web_url": "http://example.com/example/example/issues/11",
+  "time_stats": {
+    "time_estimate": 0,
+    "total_time_spent": 0,
+    "human_time_estimate": null,
+    "human_total_time_spent": null
+  },
   "confidential": false,
   "_links": {
     "self": "http://example.com/api/v4/projects/1/issues/2",
@@ -672,6 +714,12 @@ Example response:
   },
   "due_date": null,
   "web_url": "http://example.com/example/example/issues/11",
+  "time_stats": {
+    "time_estimate": 0,
+    "total_time_spent": 0,
+    "human_time_estimate": null,
+    "human_total_time_spent": null
+  },
   "confidential": false,
   "_links": {
     "self": "http://example.com/api/v4/projects/1/issues/2",
@@ -1001,7 +1049,13 @@ Example response:
     "user_notes_count": 1,
     "should_remove_source_branch": null,
     "force_remove_source_branch": false,
-    "web_url": "https://gitlab.example.com/gitlab-org/gitlab-test/merge_requests/6432"
+    "web_url": "https://gitlab.example.com/gitlab-org/gitlab-test/merge_requests/6432",
+    "time_stats": {
+      "time_estimate": 0,
+      "total_time_spent": 0,
+      "human_time_estimate": null,
+      "human_total_time_spent": null
+    }
   }
 ]
 ```
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index 802e5362d70..4f67aa4b9d4 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -92,7 +92,13 @@ Parameters:
     "user_notes_count": 1,
     "should_remove_source_branch": true,
     "force_remove_source_branch": false,
-    "web_url": "http://example.com/example/example/merge_requests/1"
+    "web_url": "http://example.com/example/example/merge_requests/1",
+    "time_stats": {
+      "time_estimate": 0,
+      "total_time_spent": 0,
+      "human_time_estimate": null,
+      "human_total_time_spent": null
+    }
   }
 ]
 ```
@@ -181,7 +187,13 @@ Parameters:
     "user_notes_count": 1,
     "should_remove_source_branch": true,
     "force_remove_source_branch": false,
-    "web_url": "http://example.com/example/example/merge_requests/1"
+    "web_url": "http://example.com/example/example/merge_requests/1",
+    "time_stats": {
+      "time_estimate": 0,
+      "total_time_spent": 0,
+      "human_time_estimate": null,
+      "human_total_time_spent": null
+    }
   }
 ]
 ```
@@ -250,7 +262,13 @@ Parameters:
   "user_notes_count": 1,
   "should_remove_source_branch": true,
   "force_remove_source_branch": false,
-  "web_url": "http://example.com/example/example/merge_requests/1"
+  "web_url": "http://example.com/example/example/merge_requests/1",
+  "time_stats": {
+    "time_estimate": 0,
+    "total_time_spent": 0,
+    "human_time_estimate": null,
+    "human_total_time_spent": null
+  }
 }
 ```
 
@@ -356,6 +374,12 @@ Parameters:
   "should_remove_source_branch": true,
   "force_remove_source_branch": false,
   "web_url": "http://example.com/example/example/merge_requests/1",
+  "time_stats": {
+    "time_estimate": 0,
+    "total_time_spent": 0,
+    "human_time_estimate": null,
+    "human_total_time_spent": null
+  }
   "changes": [
     {
     "old_path": "VERSION",
@@ -442,7 +466,13 @@ POST /projects/:id/merge_requests
   "user_notes_count": 0,
   "should_remove_source_branch": true,
   "force_remove_source_branch": false,
-  "web_url": "http://example.com/example/example/merge_requests/1"
+  "web_url": "http://example.com/example/example/merge_requests/1",
+  "time_stats": {
+    "time_estimate": 0,
+    "total_time_spent": 0,
+    "human_time_estimate": null,
+    "human_total_time_spent": null
+  }
 }
 ```
 
@@ -519,7 +549,13 @@ Must include at least one non-required attribute from above.
   "user_notes_count": 1,
   "should_remove_source_branch": true,
   "force_remove_source_branch": false,
-  "web_url": "http://example.com/example/example/merge_requests/1"
+  "web_url": "http://example.com/example/example/merge_requests/1",
+  "time_stats": {
+    "time_estimate": 0,
+    "total_time_spent": 0,
+    "human_time_estimate": null,
+    "human_total_time_spent": null
+  }
 }
 ```
 
@@ -617,7 +653,13 @@ Parameters:
   "user_notes_count": 1,
   "should_remove_source_branch": true,
   "force_remove_source_branch": false,
-  "web_url": "http://example.com/example/example/merge_requests/1"
+  "web_url": "http://example.com/example/example/merge_requests/1",
+  "time_stats": {
+    "time_estimate": 0,
+    "total_time_spent": 0,
+    "human_time_estimate": null,
+    "human_total_time_spent": null
+  }
 }
 ```
 
@@ -687,7 +729,13 @@ Parameters:
   "user_notes_count": 1,
   "should_remove_source_branch": true,
   "force_remove_source_branch": false,
-  "web_url": "http://example.com/example/example/merge_requests/1"
+  "web_url": "http://example.com/example/example/merge_requests/1",
+  "time_stats": {
+    "time_estimate": 0,
+    "total_time_spent": 0,
+    "human_time_estimate": null,
+    "human_total_time_spent": null
+  }
 }
 ```
 
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e8dd61e493f..803b48dd88a 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -320,7 +320,10 @@ module API
     end
 
     class IssueBasic < ProjectEntity
-      expose :label_names, as: :labels
+      expose :labels do |issue, options|
+        # Avoids an N+1 query since labels are preloaded
+        issue.labels.map(&:title).sort
+      end
       expose :milestone, using: Entities::Milestone
       expose :assignees, :author, using: Entities::UserBasic
 
@@ -329,13 +332,32 @@ module API
       end
 
       expose :user_notes_count
-      expose :upvotes, :downvotes
+      expose :upvotes do |issue, options|
+        if options[:issuable_metadata]
+          # Avoids an N+1 query when metadata is included
+          options[:issuable_metadata][issue.id].upvotes
+        else
+          issue.upvotes
+        end
+      end
+      expose :downvotes do |issue, options|
+        if options[:issuable_metadata]
+          # Avoids an N+1 query when metadata is included
+          options[:issuable_metadata][issue.id].downvotes
+        else
+          issue.downvotes
+        end
+      end
       expose :due_date
       expose :confidential
 
       expose :web_url do |issue, options|
         Gitlab::UrlBuilder.build(issue)
       end
+
+      expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |issue|
+        issue
+      end
     end
 
     class Issue < IssueBasic
@@ -365,10 +387,22 @@ module API
     end
 
     class IssuableTimeStats < Grape::Entity
+      format_with(:time_tracking_formatter) do |time_spent|
+        Gitlab::TimeTrackingFormatter.output(time_spent)
+      end
+
       expose :time_estimate
       expose :total_time_spent
       expose :human_time_estimate
-      expose :human_total_time_spent
+
+      with_options(format_with: :time_tracking_formatter) do
+        expose :total_time_spent, as: :human_total_time_spent
+      end
+
+      def total_time_spent
+        # Avoids an N+1 query since timelogs are preloaded
+        object.timelogs.map(&:time_spent).sum
+      end
     end
 
     class ExternalIssue < Grape::Entity
@@ -418,6 +452,10 @@ module API
       expose :web_url do |merge_request, options|
         Gitlab::UrlBuilder.build(merge_request)
       end
+
+      expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |merge_request|
+        merge_request
+      end
     end
 
     class MergeRequest < MergeRequestBasic
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 6503629e2a2..0297023226f 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -4,6 +4,8 @@ module API
 
     before { authenticate! }
 
+    helpers ::Gitlab::IssuableMetadata
+
     helpers do
       def find_issues(args = {})
         args = params.merge(args)
@@ -13,6 +15,7 @@ module API
         args[:label_name] = args.delete(:labels)
 
         issues = IssuesFinder.new(current_user, args).execute
+          .preload(:assignees, :labels, :notes, :timelogs)
 
         issues.reorder(args[:order_by] => args[:sort])
       end
@@ -65,7 +68,13 @@ module API
       get do
         issues = find_issues
 
-        present paginate(issues), with: Entities::IssueBasic, current_user: current_user
+        options = {
+          with: Entities::IssueBasic,
+          current_user: current_user,
+          issuable_metadata: issuable_meta_data(issues, 'Issue')
+        }
+
+        present paginate(issues), options
       end
     end
 
@@ -86,7 +95,13 @@ module API
 
         issues = find_issues(group_id: group.id)
 
-        present paginate(issues), with: Entities::IssueBasic, current_user: current_user
+        options = {
+          with: Entities::IssueBasic,
+          current_user: current_user,
+          issuable_metadata: issuable_meta_data(issues, 'Issue')
+        }
+
+        present paginate(issues), options
       end
     end
 
@@ -109,7 +124,14 @@ module API
 
         issues = find_issues(project_id: project.id)
 
-        present paginate(issues), with: Entities::IssueBasic, current_user: current_user, project: user_project
+        options = {
+          with: Entities::IssueBasic,
+          current_user: current_user,
+          project: user_project,
+          issuable_metadata: issuable_meta_data(issues, 'Issue')
+        }
+
+        present paginate(issues), options
       end
 
       desc 'Get a single project issue' do
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 969c6064662..eec8d9357aa 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -21,7 +21,7 @@ module API
         return merge_requests if args[:view] == 'simple'
 
         merge_requests
-          .preload(:notes, :author, :assignee, :milestone, :merge_request_diff, :labels)
+          .preload(:notes, :author, :assignee, :milestone, :merge_request_diff, :labels, :timelogs)
       end
 
       params :merge_requests_params do
diff --git a/spec/fixtures/api/schemas/public_api/v4/issues.json b/spec/fixtures/api/schemas/public_api/v4/issues.json
index bd6bfc03199..8acd9488215 100644
--- a/spec/fixtures/api/schemas/public_api/v4/issues.json
+++ b/spec/fixtures/api/schemas/public_api/v4/issues.json
@@ -78,7 +78,13 @@
       "downvotes": { "type": "integer" },
       "due_date": { "type": ["date", "null"] },
       "confidential": { "type": "boolean" },
-      "web_url": { "type": "uri" }
+      "web_url": { "type": "uri" },
+      "time_stats": {
+        "time_estimate": { "type": "integer" },
+        "total_time_spent": { "type": "integer" },
+        "human_time_estimate": { "type": ["string", "null"] },
+        "human_total_time_spent": { "type": ["string", "null"] }
+      }
     },
     "required": [
       "id", "iid", "project_id", "title", "description",
diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json
index 60aa47c1259..31b3f4ba946 100644
--- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json
+++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json
@@ -72,7 +72,13 @@
       "user_notes_count": { "type": "integer" },
       "should_remove_source_branch": { "type": ["boolean", "null"] },
       "force_remove_source_branch": { "type": ["boolean", "null"] },
-      "web_url": { "type": "uri" }
+      "web_url": { "type": "uri" },
+      "time_stats": {
+        "time_estimate": { "type": "integer" },
+        "total_time_spent": { "type": "integer" },
+        "human_time_estimate": { "type": ["string", "null"] },
+        "human_total_time_spent": { "type": ["string", "null"] }
+      }
     },
     "required": [
       "id", "iid", "project_id", "title", "description",
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 9a0c62467d3..dee75c96b86 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -509,6 +509,18 @@ describe API::Issues, :mailer do
   describe "GET /projects/:id/issues" do
     let(:base_url) { "/projects/#{project.id}" }
 
+    it 'avoids N+1 queries' do
+      control_count = ActiveRecord::QueryRecorder.new do
+        get api("/projects/#{project.id}/issues", user)
+      end.count
+
+      create(:issue, author: user, project: project)
+
+      expect do
+        get api("/projects/#{project.id}/issues", user)
+      end.not_to exceed_query_limit(control_count)
+    end
+
     it 'returns 404 when project does not exist' do
       get api('/projects/1000/issues', non_member)