Commit 0e99daae authored by Patrick Derichs's avatar Patrick Derichs

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
parent 533237a0
......@@ -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(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
}.tap { |hash| hash[:project] = project if respond_to?(:project, true) }
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
end
......@@ -243,7 +243,7 @@ module NotesActions
end
def notes_finder
@notes_finder ||= NotesFinder.new(project, current_user, finder_params)
@notes_finder ||= NotesFinder.new(current_user, finder_params)
end
def note_serializer
......
......@@ -68,7 +68,7 @@ class Projects::NotesController < Projects::ApplicationController
alias_method :awardable, :note
def finder_params
params.merge(last_fetched_at: last_fetched_at, notes_filter: notes_filter)
params.merge(project: project, last_fetched_at: last_fetched_at, notes_filter: notes_filter)
end
def authorize_admin_note!
......
......@@ -27,7 +27,9 @@ class Snippets::NotesController < ApplicationController
alias_method :noteable, :snippet
def finder_params
params.merge(last_fetched_at: last_fetched_at, target_id: snippet.id, target_type: 'personal_snippet')
params.merge(last_fetched_at: last_fetched_at, target_id: snippet.id, target_type: 'personal_snippet').tap do |hash|
hash[:project] = project if respond_to?(:project)
end
end
def authorize_read_snippet!
......
......@@ -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,15 +12,17 @@ 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
# search: string
#
def initialize(project, current_user, params = {})
@project = project
def initialize(current_user, params = {})
@project = params[: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)
......
......@@ -76,7 +76,7 @@ module API
def find_noteable(parent_type, parent_id, noteable_type, noteable_id)
params = params_by_noteable_type_and_id(noteable_type, noteable_id)
noteable = NotesFinder.new(user_project, current_user, params).target
noteable = NotesFinder.new(current_user, params.merge(project: user_project)).target
noteable = nil unless can?(current_user, noteable_read_ability_name(noteable), noteable)
noteable || not_found!(noteable_type)
end
......
......@@ -108,7 +108,7 @@ module Gitlab
# rubocop: disable CodeReuse/ActiveRecord
def notes_finder(type)
NotesFinder.new(project, @current_user, search: query, target_type: type).execute.user.order('updated_at DESC')
NotesFinder.new(@current_user, search: query, target_type: type, project: project).execute.user.order('updated_at DESC')
end
# rubocop: enable CodeReuse/ActiveRecord
......
# 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
......@@ -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 }
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
{
"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
}
}
}
}
{
"type": "array",
"items": { "$ref": "discussion.json" }
}
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment