From 355a2df560dea2719a027ada718a923ab7876ffe Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Fri, 26 Jul 2019 09:10:27 +0200 Subject: [PATCH] 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 | 22 ++++-- app/finders/notes_finder.rb | 38 +++++++--- .../concerns/issuable_actions_spec.rb | 69 +++++++++++++++++++ .../projects/issues_controller_spec.rb | 22 ++++++ .../merge_requests_controller_spec.rb | 16 +++++ spec/finders/notes_finder_spec.rb | 31 +++++++-- .../api/schemas/entities/discussion.json | 67 ++++++++++++++++++ .../api/schemas/entities/discussions.json | 4 ++ .../discussions_provider_shared_examples.rb | 15 ++++ 9 files changed, 260 insertions(+), 24 deletions(-) create mode 100644 spec/controllers/concerns/issuable_actions_spec.rb create mode 100644 spec/fixtures/api/schemas/entities/discussion.json create mode 100644 spec/fixtures/api/schemas/entities/discussions.json create mode 100644 spec/support/shared_examples/discussions_provider_shared_examples.rb 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