Merge branch 'ee-41967_issue_api_closed_by_info' into 'master'

[port https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17042] Add closed by information to issue API

See merge request gitlab-org/gitlab-ee!4855

app/models/issue.rb

@@ -27,6 +27,7 @@ class Issue < ActiveRecord::Base
 
   belongs_to :project
   belongs_to :moved_to, class_name: 'Issue'
+  belongs_to :closed_by, class_name: 'User'
 
   has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.issues&.maximum(:iid) }
 
@@ -85,6 +86,11 @@ class Issue < ActiveRecord::Base
     before_transition any => :closed do |issue|
       issue.closed_at = Time.zone.now
     end
+
+    before_transition closed: :opened do |issue|
+      issue.closed_at = nil
+      issue.closed_by = nil
+    end
   end
 
   class << self

app/services/issues/close_service.rb

@@ -23,6 +23,7 @@ module Issues
       end
 
       if project.issues_enabled? && issue.close
+        issue.update(closed_by: current_user)
         event_service.close_issue(issue, current_user)
         create_note(issue, commit) if system_note
         notification_service.close_issue(issue, current_user) if notifications

changelogs/unreleased/41967_issue_api_closed_by_info.yml

+---
+title: adds closed by informations in issue api
+merge_request: 17042
+author: haseebeqx
+type: added

db/migrate/20180209165249_add_closed_by_to_issues.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddClosedByToIssues < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def up
+    add_column :issues, :closed_by_id, :integer
+    add_concurrent_foreign_key :issues, :users, column: :closed_by_id, on_delete: :nullify
+  end
+
+  def down
+    remove_foreign_key :issues, column: :closed_by_id
+    remove_column :issues, :closed_by_id
+  end
+end

db/schema.rb

@@ -1304,6 +1304,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do
     t.integer "last_edited_by_id"
     t.boolean "discussion_locked"
     t.datetime_with_timezone "closed_at"
+    t.integer "closed_by_id"
   end
 
   add_index "issues", ["author_id"], name: "index_issues_on_author_id", using: :btree
@@ -2707,6 +2708,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do
   add_foreign_key "issues", "milestones", name: "fk_96b1dd429c", on_delete: :nullify
   add_foreign_key "issues", "projects", name: "fk_899c8f3231", on_delete: :cascade
   add_foreign_key "issues", "users", column: "author_id", name: "fk_05f1e72feb", on_delete: :nullify
+  add_foreign_key "issues", "users", column: "closed_by_id", name: "fk_c63cbf6c25", on_delete: :nullify
   add_foreign_key "issues", "users", column: "updated_by_id", name: "fk_ffed080f01", on_delete: :nullify
   add_foreign_key "label_priorities", "labels", on_delete: :cascade
   add_foreign_key "label_priorities", "projects", on_delete: :cascade

doc/api/issues.md

@@ -100,6 +100,7 @@ Example response:
       },
       "updated_at" : "2016-01-04T15:31:51.081Z",
       "closed_at" : null,
+      "closed_by" : null,
       "id" : 76,
       "title" : "Consequatur vero maxime deserunt laboriosam est voluptas dolorem.",
       "created_at" : "2016-01-04T15:31:51.081Z",
@@ -123,6 +124,8 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## List group issues
 
 Get a list of a group's issues.
@@ -217,6 +220,7 @@ Example response:
       "updated_at" : "2016-01-04T15:31:46.176Z",
       "created_at" : "2016-01-04T15:31:46.176Z",
       "closed_at" : null,
+      "closed_by" : null,
       "user_notes_count": 1,
       "due_date": null,
       "web_url": "http://example.com/example/example/issues/1",
@@ -235,6 +239,8 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## List project issues
 
 Get a list of a project's issues.
@@ -328,6 +334,14 @@ Example response:
       "updated_at" : "2016-01-04T15:31:46.176Z",
       "created_at" : "2016-01-04T15:31:46.176Z",
       "closed_at" : "2016-01-05T15:31:46.176Z",
+      "closed_by" : {
+         "state" : "active",
+         "web_url" : "https://gitlab.example.com/root",
+         "avatar_url" : null,
+         "username" : "root",
+         "id" : 1,
+         "name" : "Administrator"
+      },
       "user_notes_count": 1,
       "due_date": "2016-07-22",
       "web_url": "http://example.com/example/example/issues/1",
@@ -346,6 +360,8 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## Single issue
 
 Get a single project issue.
@@ -412,6 +428,8 @@ Example response:
    "title" : "Ut commodi ullam eos dolores perferendis nihil sunt.",
    "updated_at" : "2016-01-04T15:31:46.176Z",
    "created_at" : "2016-01-04T15:31:46.176Z",
+   "closed_at" : null,
+   "closed_by" : null,
    "subscribed": false,
    "user_notes_count": 1,
    "due_date": null,
@@ -436,6 +454,8 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## New issue
 
 Creates a new project issue.
@@ -489,6 +509,7 @@ Example response:
    "description" : null,
    "updated_at" : "2016-01-07T12:44:33.959Z",
    "closed_at" : null,
+   "closed_by" : null,
    "milestone" : null,
    "subscribed" : true,
    "user_notes_count": 0,
@@ -514,6 +535,8 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## Edit issue
 
 Updates an existing project issue. This call is also used to mark an issue as
@@ -562,6 +585,14 @@ Example response:
    "description" : null,
    "updated_at" : "2016-01-07T12:55:16.213Z",
    "closed_at" : "2016-01-08T12:55:16.213Z",
+   "closed_by" : {
+      "state" : "active",
+      "web_url" : "https://gitlab.example.com/root",
+      "avatar_url" : null,
+      "username" : "root",
+      "id" : 1,
+      "name" : "Administrator"
+    },
    "iid" : 15,
    "labels" : [
       "bug"
@@ -594,6 +625,8 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## Delete an issue
 
 Only for admins and project owners. Soft deletes the issue in question.
@@ -647,6 +680,7 @@ Example response:
   "created_at": "2016-04-05T21:41:45.652Z",
   "updated_at": "2016-04-07T12:20:17.596Z",
   "closed_at": null,
+  "closed_by": null,
   "labels": [],
   "milestone": null,
   "assignees": [{
@@ -695,6 +729,8 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## Subscribe to an issue
 
 Subscribes the authenticated user to an issue to receive notifications.
@@ -727,6 +763,7 @@ Example response:
   "created_at": "2016-04-05T21:41:45.652Z",
   "updated_at": "2016-04-07T12:20:17.596Z",
   "closed_at": null,
+  "closed_by": null,
   "labels": [],
   "milestone": null,
   "assignees": [{
@@ -775,6 +812,9 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## Unsubscribe from an issue
 
 Unsubscribes the authenticated user from the issue to not receive notifications
@@ -816,6 +856,8 @@ Example response:
     "avatar_url": "http://www.gravatar.com/avatar/3e6f06a86cf27fa8b56f3f74f7615987?s=80&d=identicon",
     "web_url": "https://gitlab.example.com/keyon"
   },
+  "closed_at": null,
+  "closed_by": null,
   "author": {
     "name": "Vivian Hermann",
     "username": "orville",
@@ -937,6 +979,9 @@ Example response:
 
 **Note**: `assignee` column is deprecated, now we show it as a single-sized array `assignees` to conform to the GitLab EE API.
 
+
+**Note**: The `closed_by` attribute was [introduced in GitLab 10.6][ce-17042]. This value will only be present for issues which were closed after GitLab 10.6 and when the user account that closed the issue still exists.
+
 ## Set a time estimate for an issue
 
 Sets an estimated time of work for this issue.
@@ -1122,6 +1167,8 @@ Example response:
     "assignee": null,
     "source_project_id": 1,
     "target_project_id": 1,
+    "closed_at": null,
+    "closed_by": null,
     "labels": [],
     "work_in_progress": false,
     "milestone": null,
@@ -1216,3 +1263,4 @@ Example response:
 
 [ce-13004]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13004
 [ce-14016]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14016
+[ce-17042]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17042

ee/app/models/burndown.rb

@@ -82,10 +82,33 @@ class Burndown
 
   def milestone_issues
     @milestone_issues ||=
-      @milestone.issues
-        .where("state = 'closed' OR (state = 'opened' AND closed_at IS NOT NULL)")
-        .reorder("closed_at ASC")
-        .pluck("closed_at, weight, state")
-        .map {|attrs| Issue.new(*attrs) }
+      begin
+        # We make use of `events` table to get the closed_at timestamp.
+        # `issues.closed_at` can't be used once it's nullified if the issue is
+        # reopened.
+        internal_clause =
+          ::Issue
+            .joins("LEFT OUTER JOIN events e ON issues.id = e.target_id AND e.target_type = 'Issue'")
+            .where(milestone: @milestone)
+            .where("state = 'closed' OR (state = 'opened' AND e.action = #{Event::CLOSED})") # rubocop:disable GitlabSecurity/SqlInjection
+
+        rel =
+          if Gitlab::Database.postgresql?
+            ::Issue
+              .select("*")
+              .from(internal_clause.select('DISTINCT ON (issues.id) issues.id, issues.state, issues.weight, e.created_at AS closed_at'))
+          else
+            ::Issue
+              .select("*")
+              .from(internal_clause.select('issues.id, issues.state, issues.weight, e.created_at AS closed_at'))
+              .group('id')
+              .having('closed_at = MIN(closed_at) OR closed_at IS NULL')
+          end
+
+        rel
+          .order('closed_at ASC')
+          .pluck('closed_at, weight, state')
+          .map { |attrs| Issue.new(*attrs) }
+      end
   end
 end

ee/spec/features/projects/milestones/milestone_spec.rb

@@ -13,6 +13,10 @@ describe 'Milestones on EE' do
     visit project_milestone_path(project, milestone)
   end
 
+  def close_issue(issue)
+    Issues::CloseService.new(issue.project, user, {}).execute(issue)
+  end
+
   context 'burndown charts' do
     let(:milestone) do
       create(:milestone,
@@ -36,11 +40,13 @@ describe 'Milestones on EE' do
         end
       end
 
-      context 'when any closed issues do not have closed_at value' do
+      context 'when a closed issue do not have closed events' do
         it 'shows warning' do
-          create(:issue, issue_params)
+          close_issue(create(:issue, issue_params))
+          close_issue(create(:issue, issue_params))
+
+          # Legacy issue: No closed events being created
           create(:closed_issue, issue_params)
-          create(:closed_issue, issue_params.merge(closed_at: nil))
 
           visit_milestone
 
@@ -50,10 +56,10 @@ describe 'Milestones on EE' do
         end
       end
 
-      context 'when all closed issues do not have closed_at value' do
+      context 'when all closed issues do not have closed events' do
         it 'shows warning and hides burndown' do
-          create(:closed_issue, issue_params.merge(closed_at: nil))
-          create(:closed_issue, issue_params.merge(closed_at: nil))
+          create(:closed_issue, issue_params)
+          create(:closed_issue, issue_params)
 
           visit_milestone
 
@@ -66,7 +72,7 @@ describe 'Milestones on EE' do
       context 'data is accurate' do
         it 'does not show warning' do
           create(:issue, issue_params)
-          create(:closed_issue, issue_params)
+          close_issue(create(:issue, issue_params))
 
           visit_milestone
 

ee/spec/models/burndown_spec.rb

@@ -65,9 +65,9 @@ describe Burndown do
     expect(burndown).to be_accurate
   end
 
-  context "when all closed issues does not have closed_at" do
+  context "when all closed issues does not have closed events" do
     before do
-      milestone.issues.update_all(closed_at: nil)
+      Event.where(target: milestone.issues, action: Event::CLOSED).destroy_all
     end
 
     it "considers closed_at as milestone start date" do
@@ -87,9 +87,9 @@ describe Burndown do
     end
   end
 
-  context "when one or more closed issues does not have closed_at" do
+  context "when one or more closed issues does not have a closed event" do
     before do
-      milestone.issues.closed.first.update(closed_at: nil)
+      Event.where(target: milestone.issues.closed.first, action: Event::CLOSED).destroy_all
     end
 
     it "sets attribute accurate to false" do
@@ -112,11 +112,25 @@ describe Burndown do
 
         # Close issues
         closed = issues.slice(0..count / 2)
-        closed.each(&:close)
+        closed.each { |issue| close_issue(issue) }
 
         # Reopen issues
-        closed.slice(0..count / 4).each(&:reopen)
+        reopened_issues = closed.slice(0..count / 4)
+        reopened_issues.each { |issue| reopen_issue(issue) }
+
+        # This generates an issue with multiple closing events
+        issue_closed_twice = reopened_issues.last
+        close_issue(issue_closed_twice)
+        reopen_issue(issue_closed_twice)
+      end
     end
   end
+
+  def close_issue(issue)
+    Issues::CloseService.new(issue.project, user, {}).execute(issue)
+  end
+
+  def reopen_issue(issue)
+    Issues::ReopenService.new(issue.project, user, {}).execute(issue)
   end
 end

lib/api/entities.rb

@@ -407,6 +407,7 @@ module API
 
     class IssueBasic < ProjectEntity
       expose :closed_at
+      expose :closed_by, using: Entities::UserBasic
       expose :labels do |issue, options|
         # Avoids an N+1 query since labels are preloaded
         issue.labels.map(&:title).sort

lib/gitlab/import_export/relation_factory.rb

@@ -19,7 +19,7 @@ module Gitlab
                     custom_attributes: 'ProjectCustomAttribute',
                     project_badges: 'Badge' }.freeze
 
-      USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id created_by_id last_edited_by_id merge_user_id resolved_by_id].freeze
+      USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id created_by_id last_edited_by_id merge_user_id resolved_by_id closed_by_id].freeze
 
       PROJECT_REFERENCES = %w[project_id source_project_id target_project_id].freeze
 

spec/lib/gitlab/import_export/all_models.yml

@@ -20,6 +20,7 @@ issues:
 - issue_assignees
 - epic_issue
 - epic
+- closed_by
 events:
 - author
 - project

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -15,6 +15,7 @@ Issue:
 - updated_by_id
 - confidential
 - closed_at
+- closed_by_id
 - due_date
 - moved_to_id
 - lock_version

spec/services/issues/close_service_spec.rb

@@ -67,6 +67,10 @@ describe Issues::CloseService do
         expect(issue).to be_closed
       end
 
+      it 'records closed user' do
+        expect(issue.closed_by_id).to be(user.id)
+      end
+
       it 'sends email to user2 about assign of new issue' do
         email = ActionMailer::Base.deliveries.last
         expect(email.to.first).to eq(user2.email)