From 7bc048616217b6cd1023c2972f882cf5d4a20309 Mon Sep 17 00:00:00 2001
From: sue445 <sue445@sue445.net>
Date: Tue, 8 Aug 2017 11:31:55 +0000
Subject: [PATCH] Expose noteable_iid in Note

---
 .../13265-project_events_noteable_iid.yml     |  4 ++
 doc/api/events.md                             | 39 +++++++++++++++++++
 doc/api/notes.md                              |  9 +++--
 lib/api/entities.rb                           |  6 +++
 spec/requests/api/events_spec.rb              | 35 +++++++++++++++++
 5 files changed, 90 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/unreleased/13265-project_events_noteable_iid.yml

diff --git a/changelogs/unreleased/13265-project_events_noteable_iid.yml b/changelogs/unreleased/13265-project_events_noteable_iid.yml
new file mode 100644
index 00000000000..54d538bb548
--- /dev/null
+++ b/changelogs/unreleased/13265-project_events_noteable_iid.yml
@@ -0,0 +1,4 @@
+---
+title: Expose noteable_iid in Note
+merge_request: 13265
+author: sue445
diff --git a/doc/api/events.md b/doc/api/events.md
index 6e530317f6c..3d5170f3f1e 100644
--- a/doc/api/events.md
+++ b/doc/api/events.md
@@ -338,6 +338,45 @@ Example response:
       "web_url":"https://gitlab.example.com/ted"
     },
     "author_username":"ted"
+  },
+  {
+    "title": null,
+    "project_id": 1,
+    "action_name": "commented on",
+    "target_id": 1312,
+    "target_iid": 1312,
+    "target_type": "Note",
+    "author_id": 1,
+    "data": null,
+    "target_title": null,
+    "created_at": "2015-12-04T10:33:58.089Z",
+    "note": {
+      "id": 1312,
+      "body": "What an awesome day!",
+      "attachment": null,
+      "author": {
+        "name": "Dmitriy Zaporozhets",
+        "username": "root",
+        "id": 1,
+        "state": "active",
+        "avatar_url": "http://localhost:3000/uploads/user/avatar/1/fox_avatar.png",
+        "web_url": "http://localhost:3000/root"
+      },
+      "created_at": "2015-12-04T10:33:56.698Z",
+      "system": false,
+      "noteable_id": 377,
+      "noteable_type": "Issue",
+      "noteable_iid": 377
+    },
+    "author": {
+      "name": "Dmitriy Zaporozhets",
+      "username": "root",
+      "id": 1,
+      "state": "active",
+      "avatar_url": "http://localhost:3000/uploads/user/avatar/1/fox_avatar.png",
+      "web_url": "http://localhost:3000/root"
+    },
+    "author_username": "root"
   }
 ]
 ```
diff --git a/doc/api/notes.md b/doc/api/notes.md
index 388e6989df2..e627369e17b 100644
--- a/doc/api/notes.md
+++ b/doc/api/notes.md
@@ -35,7 +35,8 @@ Parameters:
     "updated_at": "2013-10-02T10:22:45Z",
     "system": true,
     "noteable_id": 377,
-    "noteable_type": "Issue"
+    "noteable_type": "Issue",
+    "noteable_iid": 377
   },
   {
     "id": 305,
@@ -53,7 +54,8 @@ Parameters:
     "updated_at": "2013-10-02T09:56:03Z",
     "system": true,
     "noteable_id": 121,
-    "noteable_type": "Issue"
+    "noteable_type": "Issue",
+    "noteable_iid": 121
   }
 ]
 ```
@@ -267,7 +269,8 @@ Parameters:
   "updated_at": "2013-10-02T08:57:14Z",
   "system": false,
   "noteable_id": 2,
-  "noteable_type": "MergeRequest"
+  "noteable_type": "MergeRequest",
+  "noteable_iid": 2
 }
 ```
 
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index bdea308ad5d..6ba4005dd0b 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -454,6 +454,9 @@ module API
     end
 
     class Note < Grape::Entity
+      # Only Issue and MergeRequest have iid
+      NOTEABLE_TYPES_WITH_IID = %w(Issue MergeRequest).freeze
+
       expose :id
       expose :note, as: :body
       expose :attachment_identifier, as: :attachment
@@ -461,6 +464,9 @@ module API
       expose :created_at, :updated_at
       expose :system?, as: :system
       expose :noteable_id, :noteable_type
+
+      # Avoid N+1 queries as much as possible
+      expose(:noteable_iid) { |note| note.noteable.iid if NOTEABLE_TYPES_WITH_IID.include?(note.noteable_type) }
     end
 
     class AwardEmoji < Grape::Entity
diff --git a/spec/requests/api/events_spec.rb b/spec/requests/api/events_spec.rb
index 7a847442469..48db964d782 100644
--- a/spec/requests/api/events_spec.rb
+++ b/spec/requests/api/events_spec.rb
@@ -138,5 +138,40 @@ describe API::Events do
         expect(response).to have_http_status(404)
       end
     end
+
+    context 'when exists some events' do
+      before do
+        create_event(note1)
+        create_event(note2)
+        create_event(merge_request1)
+      end
+
+      let(:note1) { create(:note_on_merge_request, project: private_project, author: user) }
+      let(:note2) { create(:note_on_issue, project: private_project, author: user) }
+      let(:merge_request1) { create(:merge_request, state: 'closed', author: user, assignee: user, source_project: private_project, title: 'Test') }
+      let(:merge_request2) { create(:merge_request, state: 'closed', author: user, assignee: user, source_project: private_project, title: 'Test') }
+
+      it 'avoids N+1 queries' do
+        control_count = ActiveRecord::QueryRecorder.new do
+          get api("/projects/#{private_project.id}/events", user)
+        end.count
+
+        create_event(merge_request2)
+
+        expect do
+          get api("/projects/#{private_project.id}/events", user)
+        end.not_to exceed_query_limit(control_count)
+
+        expect(response).to have_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(json_response[0]).to include('target_type' => 'MergeRequest', 'target_id' => merge_request2.id)
+        expect(json_response[1]).to include('target_type' => 'MergeRequest', 'target_id' => merge_request1.id)
+      end
+
+      def create_event(target)
+        create(:event, project: private_project, author: user, target: target)
+      end
+    end
   end
 end