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

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

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)

spec/controllers/concerns/issuable_actions_spec.rb

+# 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

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 }

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

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

spec/fixtures/api/schemas/entities/discussion.json

+{
+  "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
+      }
+    }
+  }
+}

spec/fixtures/api/schemas/entities/discussions.json

+{
+  "type": "array",
+  "items": { "$ref": "discussion.json" }
+}

spec/support/shared_examples/discussions_provider_shared_examples.rb

+# 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