Use NotesFinder in module IssuableActions

Add spec for concern IssuableActions

Add shared samples for discussions endpoint

Add schema validations for discussions

Fix rubocop style issue

Make target assignable

Use new possibility to provide target

9 files changed, 260 insertions, 24 deletions

diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index 6fa2f75be33..94d1e8248fc 100644
--- a/app/controllers/concerns/issuable_actions.rb
+++ b/app/controllers/concerns/issuable_actions.rb
@@ -98,13 +98,12 @@ module IssuableActions
     render json: { notice: "#{quantity} #{resource_name.pluralize(quantity)} updated" }
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
+  # rubocop:disable CodeReuse/ActiveRecord
   def discussions
-    notes = issuable.discussion_notes
-      .inc_relations_for_view
-      .with_notes_filter(notes_filter)
-      .includes(:noteable)
-      .fresh
+    notes = NotesFinder.new(project, current_user, finder_params_for_issuable).execute
+                .inc_relations_for_view
+                .includes(:noteable)
+                .fresh
 
     if notes_filter != UserPreference::NOTES_FILTERS[:only_comments]
       notes = ResourceEvents::MergeIntoNotesService.new(issuable, current_user).execute(notes)
@@ -117,7 +116,7 @@ module IssuableActions
 
     render json: discussion_serializer.represent(discussions, context: self)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
+  # rubocop:enable CodeReuse/ActiveRecord
 
   private
 
@@ -222,4 +221,13 @@ module IssuableActions
   def parent
     @project || @group # rubocop:disable Gitlab/ModuleWithInstanceVariables
   end
+
+  # rubocop:disable Gitlab/ModuleWithInstanceVariables
+  def finder_params_for_issuable
+    {
+        target: @issuable,
+        notes_filter: notes_filter
+    }
+  end
+  # rubocop:enable Gitlab/ModuleWithInstanceVariables
 end
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 8f610d7dddb..c25600221d7 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -3,6 +3,8 @@
 class NotesFinder
   FETCH_OVERLAP = 5.seconds
 
+  attr_reader :target_type
+
   # Used to filter Notes
   # When used with target_type and target_id this returns notes specifically for the controller
   #
@@ -10,6 +12,7 @@ class NotesFinder
   #   current_user - which user check authorizations with
   #   project - which project to look for notes on
   #   params:
+  #     target: noteable
   #     target_type: string
   #     target_id: integer
   #     last_fetched_at: time
@@ -18,7 +21,8 @@ class NotesFinder
   def initialize(project, current_user, params = {})
     @project = project
     @current_user = current_user
-    @params = params
+    @params = params.dup
+    @target_type = @params[:target_type]
   end
 
   def execute
@@ -32,7 +36,27 @@ class NotesFinder
   def target
     return @target if defined?(@target)
 
-    target_type = @params[:target_type]
+    if target_given?
+      use_explicit_target
+    else
+      find_target_by_type_and_ids
+    end
+  end
+
+  private
+
+  def target_given?
+    @params.key?(:target)
+  end
+
+  def use_explicit_target
+    @target = @params[:target]
+    @target_type = @target.class.name.underscore
+
+    @target
+  end
+
+  def find_target_by_type_and_ids
     target_id   = @params[:target_id]
     target_iid  = @params[:target_iid]
 
@@ -45,13 +69,11 @@ class NotesFinder
           @project.commit(target_id)
         end
       else
-        noteables_for_type_by_id(target_type, target_id, target_iid)
+        noteable_for_type_by_id(target_type, target_id, target_iid)
       end
   end
 
-  private
-
-  def noteables_for_type_by_id(type, id, iid)
+  def noteable_for_type_by_id(type, id, iid)
     query = if id
               { id: id }
             else
@@ -77,10 +99,6 @@ class NotesFinder
     search(notes)
   end
 
-  def target_type
-    @params[:target_type]
-  end
-
   # rubocop: disable CodeReuse/ActiveRecord
   def notes_of_any_type
     types = %w(commit issue merge_request snippet)
diff --git a/spec/controllers/concerns/issuable_actions_spec.rb b/spec/controllers/concerns/issuable_actions_spec.rb
new file mode 100644
index 00000000000..3281fd4a2ef
--- /dev/null
+++ b/spec/controllers/concerns/issuable_actions_spec.rb
@@ -0,0 +1,69 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe IssuableActions do
+  let(:project) { double('project') }
+  let(:user) { double('user') }
+  let(:issuable) { double('issuable') }
+  let(:finder_params_for_issuable) { {} }
+  let(:notes_result) { double('notes_result') }
+  let(:discussion_serializer) { double('discussion_serializer') }
+
+  let(:controller) do
+    klass = Class.new do
+      attr_reader :current_user, :project, :issuable
+
+      def self.before_action(action, params = nil)
+      end
+
+      include IssuableActions
+
+      def initialize(issuable, project, user, finder_params)
+        @issuable = issuable
+        @project = project
+        @current_user = user
+        @finder_params = finder_params
+      end
+
+      def finder_params_for_issuable
+        @finder_params
+      end
+
+      def params
+        {
+          notes_filter: 1
+        }
+      end
+
+      def prepare_notes_for_rendering(notes)
+        []
+      end
+
+      def render(options)
+      end
+    end
+
+    klass.new(issuable, project, user, finder_params_for_issuable)
+  end
+
+  describe '#discussions' do
+    before do
+      allow(user).to receive(:set_notes_filter)
+      allow(user).to receive(:user_preference)
+      allow(discussion_serializer).to receive(:represent)
+    end
+
+    it 'instantiates and calls NotesFinder as expected' do
+      expect(Discussion).to receive(:build_collection).and_return([])
+      expect(DiscussionSerializer).to receive(:new).and_return(discussion_serializer)
+      expect(NotesFinder).to receive(:new).with(project, user, finder_params_for_issuable).and_call_original
+
+      expect_any_instance_of(NotesFinder).to receive(:execute).and_return(notes_result)
+
+      expect(notes_result).to receive_messages(inc_relations_for_view: notes_result, includes: notes_result, fresh: notes_result)
+
+      controller.discussions
+    end
+  end
+end
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 32d14dce936..0f885d776e1 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -1260,6 +1260,28 @@ describe Projects::IssuesController do
         sign_in(user)
       end
 
+      context do
+        it_behaves_like 'discussions provider' do
+          let!(:author) { create(:user) }
+          let!(:project) { create(:project) }
+
+          let!(:issue) { create(:issue, project: project, author: user) }
+
+          let!(:note_on_issue1) { create(:discussion_note_on_issue, noteable: issue, project: issue.project, author: create(:user)) }
+          let!(:note_on_issue2) { create(:discussion_note_on_issue, noteable: issue, project: issue.project, author: create(:user)) }
+
+          let(:requested_iid) { issue.iid }
+          let(:expected_discussion_count) { 3 }
+          let(:expected_discussion_ids) do
+            [
+              issue.notes.first.discussion_id,
+              note_on_issue1.discussion_id,
+              note_on_issue2.discussion_id
+            ]
+          end
+        end
+      end
+
       it 'returns discussion json' do
         get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
 
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index f11880122b1..b777fa7f816 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -1121,6 +1121,22 @@ describe Projects::MergeRequestsController do
         end
       end
     end
+
+    context do
+      it_behaves_like 'discussions provider' do
+        let!(:author) { create(:user) }
+        let!(:project) { create(:project) }
+
+        let!(:merge_request) { create(:merge_request, source_project: project) }
+
+        let!(:mr_note1) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+        let!(:mr_note2) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+
+        let(:requested_iid) { merge_request.iid }
+        let(:expected_discussion_count) { 2 }
+        let(:expected_discussion_ids) { [mr_note1.discussion_id, mr_note2.discussion_id] }
+      end
+    end
   end
 
   describe 'GET edit' do
diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
index 87bde4ca2f6..677fde0d8ce 100644
--- a/spec/finders/notes_finder_spec.rb
+++ b/spec/finders/notes_finder_spec.rb
@@ -118,16 +118,11 @@ describe NotesFinder do
 
     context 'for target' do
       let(:project) { create(:project, :repository) }
-      let(:note1) { create :note_on_commit, project: project }
-      let(:note2) { create :note_on_commit, project: project }
+      let!(:note1) { create :note_on_commit, project: project }
+      let!(:note2) { create :note_on_commit, project: project }
       let(:commit) { note1.noteable }
       let(:params) { { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago.to_i } }
 
-      before do
-        note1
-        note2
-      end
-
       it 'finds all notes' do
         notes = described_class.new(project, user, params).execute
         expect(notes.size).to eq(2)
@@ -194,6 +189,28 @@ describe NotesFinder do
         end
       end
     end
+
+    context 'for explicit target' do
+      let(:project) { create(:project, :repository) }
+      let!(:note1) { create :note_on_commit, project: project, created_at: 1.day.ago, updated_at: 2.hours.ago }
+      let!(:note2) { create :note_on_commit, project: project }
+      let(:commit) { note1.noteable }
+      let(:params) { { target: commit } }
+
+      it 'returns the expected notes' do
+        expect(described_class.new(project, user, params).execute).to eq([note1, note2])
+      end
+
+      it 'returns the expected notes when last_fetched_at is given' do
+        params = { target: commit, last_fetched_at: 1.hour.ago.to_i }
+        expect(described_class.new(project, user, params).execute).to eq([note2])
+      end
+
+      it 'fails when nil is provided' do
+        params = { target: nil }
+        expect { described_class.new(project, user, params).execute }.to raise_error(RuntimeError)
+      end
+    end
   end
 
   describe '.search' do
diff --git a/spec/fixtures/api/schemas/entities/discussion.json b/spec/fixtures/api/schemas/entities/discussion.json
new file mode 100644
index 00000000000..bcc1db79e83
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/discussion.json
@@ -0,0 +1,67 @@
+{
+  "type": "object",
+  "required" : [
+    "id",
+    "notes",
+    "individual_note"
+  ],
+  "properties" : {
+    "id": { "type": "string" },
+    "individual_note": { "type": "boolean" },
+    "notes": {
+      "type": "array",
+      "items": {
+        "type": "object",
+        "properties" : {
+          "id": { "type": "string" },
+          "type": { "type": ["string", "null"] },
+          "body": { "type": "string" },
+          "attachment": { "type": ["string", "null"]},
+          "award_emoji": { "type": "array" },
+          "author": {
+            "type": "object",
+            "properties": {
+              "name": { "type": "string" },
+              "username": { "type": "string" },
+              "id": { "type": "integer" },
+              "state": { "type": "string" },
+              "avatar_url": { "type": "uri" },
+              "web_url": { "type": "uri" },
+              "status_tooltip_html": { "type": ["string", "null"] },
+              "path": { "type": "string" }
+            },
+            "additionalProperties": false
+          },
+          "created_at": { "type": "date" },
+          "updated_at": { "type": "date" },
+          "system": { "type": "boolean" },
+          "noteable_id": { "type": "integer" },
+          "noteable_iid": { "type": "integer" },
+          "noteable_type": { "type": "string" },
+          "resolved": { "type": "boolean" },
+          "resolvable": { "type": "boolean" },
+          "resolved_by": { "type": ["string", "null"] },
+          "note": { "type": "string" },
+          "note_html": { "type": "string"  },
+          "current_user": { "type": "object" },
+          "suggestions": { "type": "array" },
+          "discussion_id": { "type": "string" },
+          "emoji_awardable": { "type": "boolean" },
+          "report_abuse_path": { "type": "string" },
+          "noteable_note_url": { "type": "string" },
+          "resolve_path": { "type": "string" },
+          "resolve_with_issue_path": { "type": "string" },
+          "cached_markdown_version": { "type": "integer" },
+          "human_access": { "type": ["string", "null"] },
+          "toggle_award_path": { "type": "string" },
+          "path": { "type": "string" }
+        },
+        "required": [
+          "id", "attachment", "author", "created_at", "updated_at",
+          "system", "noteable_id", "noteable_type"
+        ],
+        "additionalProperties": false
+      }
+    }
+  }
+}
diff --git a/spec/fixtures/api/schemas/entities/discussions.json b/spec/fixtures/api/schemas/entities/discussions.json
new file mode 100644
index 00000000000..5a837429776
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/discussions.json
@@ -0,0 +1,4 @@
+{
+  "type": "array",
+  "items": { "$ref": "discussion.json" }
+}
diff --git a/spec/support/shared_examples/discussions_provider_shared_examples.rb b/spec/support/shared_examples/discussions_provider_shared_examples.rb
new file mode 100644
index 00000000000..77cf1ac3f51
--- /dev/null
+++ b/spec/support/shared_examples/discussions_provider_shared_examples.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+shared_examples 'discussions provider' do
+  it 'returns the expected discussions' do
+    get :discussions, params: { namespace_id: project.namespace, project_id: project, id: requested_iid }
+
+    expect(response).to have_gitlab_http_status(200)
+    expect(response).to match_response_schema('entities/discussions')
+
+    expect(json_response.size).to eq(expected_discussion_count)
+    expect(json_response.pluck('id')).to eq(expected_discussion_ids)
+  end
+end