diff options
author | Patrick Derichs <pderichs@gitlab.com> | 2019-07-30 20:25:49 +0200 |
---|---|---|
committer | Patrick Derichs <pderichs@gitlab.com> | 2019-08-01 10:42:42 +0200 |
commit | 0e99daae4afdb90d74c4b0bfe5cb3e482bbb422e (patch) | |
tree | f6170c10e6e6e657425c3d484eabd04d57c87bb0 /spec | |
parent | 533237a097281dbe4fb1c821d5823c4de8c8f6af (diff) | |
download | gitlab-ce-0e99daae4afdb90d74c4b0bfe5cb3e482bbb422e.tar.gz |
Use NotesFinder in IssuableActions module
Remove project from NotesFinder constructor
Add project parameter to specs
Also look for methods in private scope
Fix specs to match new NotesFinder constructor
Diffstat (limited to 'spec')
8 files changed, 264 insertions, 54 deletions
diff --git a/spec/controllers/concerns/issuable_actions_spec.rb b/spec/controllers/concerns/issuable_actions_spec.rb new file mode 100644 index 00000000000..7b0b4497f3f --- /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(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 57a432de3f5..b1dc6a65dd4 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -1210,6 +1210,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/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 98aea9056dc..9ab565dc2e8 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -43,7 +43,7 @@ describe Projects::NotesController do request.headers['X-Last-Fetched-At'] = last_fetched_at expect(NotesFinder).to receive(:new) - .with(anything, anything, hash_including(last_fetched_at: last_fetched_at)) + .with(anything, hash_including(last_fetched_at: last_fetched_at)) .and_call_original get :index, params: request_params diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 87bde4ca2f6..88906adfeeb 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -14,7 +14,7 @@ describe NotesFinder do let!(:system_note) { create(:note_on_issue, project: project, system: true) } it 'returns only user notes when using only_comments filter' do - finder = described_class.new(project, user, notes_filter: UserPreference::NOTES_FILTERS[:only_comments]) + finder = described_class.new(user, project: project, notes_filter: UserPreference::NOTES_FILTERS[:only_comments]) notes = finder.execute @@ -22,7 +22,7 @@ describe NotesFinder do end it 'returns only system notes when using only_activity filters' do - finder = described_class.new(project, user, notes_filter: UserPreference::NOTES_FILTERS[:only_activity]) + finder = described_class.new(user, project: project, notes_filter: UserPreference::NOTES_FILTERS[:only_activity]) notes = finder.execute @@ -30,7 +30,7 @@ describe NotesFinder do end it 'gets all notes' do - finder = described_class.new(project, user, notes_filter: UserPreference::NOTES_FILTERS[:all_activity]) + finder = described_class.new(user, project: project, notes_filter: UserPreference::NOTES_FILTERS[:all_activity]) notes = finder.execute @@ -41,7 +41,7 @@ describe NotesFinder do it 'finds notes on merge requests' do create(:note_on_merge_request, project: project) - notes = described_class.new(project, user).execute + notes = described_class.new(user, project: project).execute expect(notes.count).to eq(1) end @@ -49,7 +49,7 @@ describe NotesFinder do it 'finds notes on snippets' do create(:note_on_project_snippet, project: project) - notes = described_class.new(project, user).execute + notes = described_class.new(user, project: project).execute expect(notes.count).to eq(1) end @@ -59,13 +59,13 @@ describe NotesFinder do note = create(:note_on_commit, project: project) params = { target_type: 'commit', target_id: note.noteable.id } - notes = described_class.new(project, create(:user), params).execute + notes = described_class.new(create(:user), params).execute expect(notes.count).to eq(0) end it 'succeeds when no notes found' do - notes = described_class.new(project, create(:user)).execute + notes = described_class.new(create(:user), project: project).execute expect(notes.count).to eq(0) end @@ -82,7 +82,7 @@ describe NotesFinder do it 'publicly excludes notes on merge requests' do create(:note_on_merge_request, project: project) - notes = described_class.new(project, create(:user)).execute + notes = described_class.new(create(:user), project: project).execute expect(notes.count).to eq(0) end @@ -90,7 +90,7 @@ describe NotesFinder do it 'publicly excludes notes on issues' do create(:note_on_issue, project: project) - notes = described_class.new(project, create(:user)).execute + notes = described_class.new(create(:user), project: project).execute expect(notes.count).to eq(0) end @@ -98,7 +98,7 @@ describe NotesFinder do it 'publicly excludes notes on snippets' do create(:note_on_project_snippet, project: project) - notes = described_class.new(project, create(:user)).execute + notes = described_class.new(create(:user), project: project).execute expect(notes.count).to eq(0) end @@ -110,7 +110,7 @@ describe NotesFinder do let!(:note2) { create :note_on_commit, project: project } it 'finds only notes for the selected type' do - notes = described_class.new(project, user, target_type: 'issue').execute + notes = described_class.new(user, project: project, target_type: 'issue').execute expect(notes).to eq([note1]) end @@ -118,56 +118,51 @@ 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 + let(:params) { { project: project, target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago.to_i } } it 'finds all notes' do - notes = described_class.new(project, user, params).execute + notes = described_class.new(user, params).execute expect(notes.size).to eq(2) end it 'finds notes on merge requests' do note = create(:note_on_merge_request, project: project) - params = { target_type: 'merge_request', target_id: note.noteable.id } + params = { project: project, target_type: 'merge_request', target_id: note.noteable.id } - notes = described_class.new(project, user, params).execute + notes = described_class.new(user, params).execute expect(notes).to include(note) end it 'finds notes on snippets' do note = create(:note_on_project_snippet, project: project) - params = { target_type: 'snippet', target_id: note.noteable.id } + params = { project: project, target_type: 'snippet', target_id: note.noteable.id } - notes = described_class.new(project, user, params).execute + notes = described_class.new(user, params).execute expect(notes.count).to eq(1) end it 'finds notes on personal snippets' do note = create(:note_on_personal_snippet) - params = { target_type: 'personal_snippet', target_id: note.noteable_id } + params = { project: project, target_type: 'personal_snippet', target_id: note.noteable_id } - notes = described_class.new(project, user, params).execute + notes = described_class.new(user, params).execute expect(notes.count).to eq(1) end it 'raises an exception for an invalid target_type' do params[:target_type] = 'invalid' - expect { described_class.new(project, user, params).execute }.to raise_error("invalid target_type '#{params[:target_type]}'") + expect { described_class.new(user, params).execute }.to raise_error("invalid target_type '#{params[:target_type]}'") end it 'filters out old notes' do note2.update_attribute(:updated_at, 2.hours.ago) - notes = described_class.new(project, user, params).execute + notes = described_class.new(user, params).execute expect(notes).to eq([note1]) end @@ -175,25 +170,47 @@ describe NotesFinder do let(:confidential_issue) { create(:issue, :confidential, project: project, author: user) } let!(:confidential_note) { create(:note, noteable: confidential_issue, project: confidential_issue.project) } - let(:params) { { target_id: confidential_issue.id, target_type: 'issue', last_fetched_at: 1.hour.ago.to_i } } + let(:params) { { project: confidential_issue.project, target_id: confidential_issue.id, target_type: 'issue', last_fetched_at: 1.hour.ago.to_i } } it 'returns notes if user can see the issue' do - expect(described_class.new(project, user, params).execute).to eq([confidential_note]) + expect(described_class.new(user, params).execute).to eq([confidential_note]) end it 'raises an error if user can not see the issue' do user = create(:user) - expect { described_class.new(project, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound) + expect { described_class.new(user, params).execute }.to raise_error(ActiveRecord::RecordNotFound) end it 'raises an error for project members with guest role' do user = create(:user) project.add_guest(user) - expect { described_class.new(project, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound) + expect { described_class.new(user, params).execute }.to raise_error(ActiveRecord::RecordNotFound) 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) { { project: project, target: commit } } + + it 'returns the expected notes' do + expect(described_class.new(user, params).execute).to eq([note1, note2]) + end + + it 'returns the expected notes when last_fetched_at is given' do + params = { project: project, target: commit, last_fetched_at: 1.hour.ago.to_i } + expect(described_class.new(user, params).execute).to eq([note2]) + end + + it 'fails when nil is provided' do + params = { project: project, target: nil } + expect { described_class.new(user, params).execute }.to raise_error(RuntimeError) + end + end end describe '.search' do @@ -201,17 +218,17 @@ describe NotesFinder do let(:note) { create(:note_on_issue, note: 'WoW', project: project) } it 'returns notes with matching content' do - expect(described_class.new(note.project, nil, search: note.note).execute).to eq([note]) + expect(described_class.new(nil, project: note.project, search: note.note).execute).to eq([note]) end it 'returns notes with matching content regardless of the casing' do - expect(described_class.new(note.project, nil, search: 'WOW').execute).to eq([note]) + expect(described_class.new(nil, project: note.project, search: 'WOW').execute).to eq([note]) end it 'returns commit notes user can access' do note = create(:note_on_commit, project: project) - expect(described_class.new(note.project, create(:user), search: note.note).execute).to eq([note]) + expect(described_class.new(create(:user), project: note.project, search: note.note).execute).to eq([note]) end context "confidential issues" do @@ -220,27 +237,27 @@ describe NotesFinder do let(:confidential_note) { create(:note, note: "Random", noteable: confidential_issue, project: confidential_issue.project) } it "returns notes with matching content if user can see the issue" do - expect(described_class.new(confidential_note.project, user, search: confidential_note.note).execute).to eq([confidential_note]) + expect(described_class.new(user, project: confidential_note.project, search: confidential_note.note).execute).to eq([confidential_note]) end it "does not return notes with matching content if user can not see the issue" do user = create(:user) - expect(described_class.new(confidential_note.project, user, search: confidential_note.note).execute).to be_empty + expect(described_class.new(user, project: confidential_note.project, search: confidential_note.note).execute).to be_empty end it "does not return notes with matching content for project members with guest role" do user = create(:user) project.add_guest(user) - expect(described_class.new(confidential_note.project, user, search: confidential_note.note).execute).to be_empty + expect(described_class.new(user, project: confidential_note.project, search: confidential_note.note).execute).to be_empty end it "does not return notes with matching content for unauthenticated users" do - expect(described_class.new(confidential_note.project, nil, search: confidential_note.note).execute).to be_empty + expect(described_class.new(nil, project: confidential_note.project, search: confidential_note.note).execute).to be_empty end end context 'inlines SQL filters on subqueries for performance' do - let(:sql) { described_class.new(note.project, nil, search: note.note).execute.to_sql } + let(:sql) { described_class.new(nil, project: note.project, search: note.note).execute.to_sql } let(:number_of_noteable_types) { 4 } specify 'project_id check' do @@ -254,11 +271,11 @@ describe NotesFinder do end describe '#target' do - subject { described_class.new(project, user, params) } + subject { described_class.new(user, params) } context 'for a issue target' do let(:issue) { create(:issue, project: project) } - let(:params) { { target_type: 'issue', target_id: issue.id } } + let(:params) { { project: project, target_type: 'issue', target_id: issue.id } } it 'returns the issue' do expect(subject.target).to eq(issue) @@ -267,7 +284,7 @@ describe NotesFinder do context 'for a merge request target' do let(:merge_request) { create(:merge_request, source_project: project) } - let(:params) { { target_type: 'merge_request', target_id: merge_request.id } } + let(:params) { { project: project, target_type: 'merge_request', target_id: merge_request.id } } it 'returns the merge_request' do expect(subject.target).to eq(merge_request) @@ -276,7 +293,7 @@ describe NotesFinder do context 'for a snippet target' do let(:snippet) { create(:project_snippet, project: project) } - let(:params) { { target_type: 'snippet', target_id: snippet.id } } + let(:params) { { project: project, target_type: 'snippet', target_id: snippet.id } } it 'returns the snippet' do expect(subject.target).to eq(snippet) @@ -286,7 +303,7 @@ describe NotesFinder do context 'for a commit target' do let(:project) { create(:project, :repository) } let(:commit) { project.commit } - let(:params) { { target_type: 'commit', target_id: commit.id } } + let(:params) { { project: project, target_type: 'commit', target_id: commit.id } } it 'returns the commit' do expect(subject.target).to eq(commit) @@ -298,24 +315,24 @@ describe NotesFinder do let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } it 'finds issues by iid' do - iid_params = { target_type: 'issue', target_iid: issue.iid } - expect(described_class.new(project, user, iid_params).target).to eq(issue) + iid_params = { project: project, target_type: 'issue', target_iid: issue.iid } + expect(described_class.new(user, iid_params).target).to eq(issue) end it 'finds merge requests by iid' do - iid_params = { target_type: 'merge_request', target_iid: merge_request.iid } - expect(described_class.new(project, user, iid_params).target).to eq(merge_request) + iid_params = { project: project, target_type: 'merge_request', target_iid: merge_request.iid } + expect(described_class.new(user, iid_params).target).to eq(merge_request) end it 'returns nil if both target_id and target_iid are not given' do - params_without_any_id = { target_type: 'issue' } - expect(described_class.new(project, user, params_without_any_id).target).to be_nil + params_without_any_id = { project: project, target_type: 'issue' } + expect(described_class.new(user, params_without_any_id).target).to be_nil end it 'prioritizes target_id over target_iid' do issue2 = create(:issue, project: project) - iid_params = { target_type: 'issue', target_id: issue2.id, target_iid: issue.iid } - expect(described_class.new(project, user, iid_params).target).to eq(issue2) + iid_params = { project: project, target_type: 'issue', target_id: issue2.id, target_iid: issue.iid } + expect(described_class.new(user, iid_params).target).to eq(issue2) end end end 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 |