diff options
author | Patrick Derichs <pderichs@gitlab.com> | 2019-06-15 07:10:58 +0200 |
---|---|---|
committer | Patrick Derichs <pderichs@gitlab.com> | 2019-06-19 10:56:55 +0200 |
commit | 932a9a0c77a02e1d948f45cffe5e936e915ae0bc (patch) | |
tree | cb1766d65bc00f56c83a044b6fc19712a9473f69 | |
parent | ba952d53c5782e49b59ba3e5dd89c2c1eca02c80 (diff) | |
download | gitlab-ce-932a9a0c77a02e1d948f45cffe5e936e915ae0bc.tar.gz |
Use NotesFinder to fetch notes on API and Controllers
Fix missing iid query on NotesFinder
Changed parameters of find_noteable,
so changes across a few files were needed.
MergeRequest also requires iid instead of id query
Make NotesFinder fail with RecordNotFound again
Add specs for target_iid
Using RSpec tablesyntax for target_iid specs
Revert "Using RSpec tablesyntax for target_iid specs"
This reverts commit ba45c7f569a.
Allow find_by! here
Fix variable name
Add readable check
Revert "Add readable check"
This reverts commit 9e3a1a7aa39.
Remove unnecessary assignment
Add required changes for EE
Fix parameter count
Reduce code duplication by extracting a noteable module method
The call to find_noteable was redundant so
multiple files and lines have changed in that
commit to use the newly introduced module
method `noteable`.
Replace casecmp with include check
Add parent_type parameter
Revert "Reduce code duplication by extracting
a noteable module method"
This reverts commit 8c0923babff16.
Method is no longer needed
Check whether noteable can be read by user
-rw-r--r-- | app/finders/notes_finder.rb | 15 | ||||
-rw-r--r-- | lib/api/discussions.rb | 18 | ||||
-rw-r--r-- | lib/api/helpers.rb | 5 | ||||
-rw-r--r-- | lib/api/helpers/notes_helpers.rb | 21 | ||||
-rw-r--r-- | lib/api/notes.rb | 12 | ||||
-rw-r--r-- | lib/api/resource_label_events.rb | 4 | ||||
-rw-r--r-- | spec/finders/notes_finder_spec.rb | 26 |
7 files changed, 71 insertions, 30 deletions
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 817aac8b5d5..83bf015488f 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -34,8 +34,9 @@ class NotesFinder target_type = @params[:target_type] target_id = @params[:target_id] + target_iid = @params[:target_iid] - return @target = nil unless target_type && target_id + return @target = nil unless target_type && (target_id || target_iid) @target = if target_type == "commit" @@ -43,12 +44,22 @@ class NotesFinder @project.commit(target_id) end else - noteables_for_type(target_type).find(target_id) + noteables_for_type_by_id(target_type, target_id, target_iid) end end private + def noteables_for_type_by_id(type, id, iid) + query = if id + { id: id } + else + { iid: iid } + end + + noteables_for_type(type).find_by!(query) # rubocop: disable CodeReuse/ActiveRecord + end + def init_collection if target notes_on_target diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index 693172b7d08..cc62ce22a1b 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -25,7 +25,7 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ":id/#{noteables_path}/:noteable_id/discussions" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = noteable.notes .inc_relations_for_view @@ -47,7 +47,7 @@ module API requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) if notes.empty? @@ -82,7 +82,7 @@ module API end end post ":id/#{noteables_path}/:noteable_id/discussions" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) type = params[:position] ? 'DiffNote' : 'DiscussionNote' id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id @@ -112,7 +112,7 @@ module API requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) if notes.empty? @@ -132,7 +132,7 @@ module API optional :created_at, type: String, desc: 'The creation date of the note' end post ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) first_note = notes.first @@ -166,7 +166,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) get_note(noteable, params[:note_id]) end @@ -183,7 +183,7 @@ module API exactly_one_of :body, :resolved end put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) if params[:resolved].nil? update_note(noteable, params[:note_id]) @@ -201,7 +201,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end delete ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) delete_note(noteable, params[:note_id]) end @@ -216,7 +216,7 @@ module API requires :resolved, type: Boolean, desc: 'Mark discussion resolved/unresolved' end put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) resolve_discussion(noteable, params[:discussion_id], params[:resolved]) end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 00bcf6b055b..70b5064c72a 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -183,11 +183,6 @@ module API user_project.commit_by(oid: id) end - def find_project_snippet(id) - finder_params = { project: user_project } - SnippetsFinder.new(current_user, finder_params).find(id) - end - # rubocop: disable CodeReuse/ActiveRecord def find_merge_request_with_access(iid, access_level = :read_merge_request) merge_request = user_project.merge_requests.find_by!(iid: iid) diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index a068de4361c..c444766249d 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -73,16 +73,27 @@ module API "read_#{noteable.class.to_s.underscore}".to_sym end - def find_noteable(parent, noteables_str, noteable_id) - noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend + def find_noteable(parent_type, parent_id, noteable_type, noteable_id) + params = params_by_noteable_type_and_id(noteable_type, noteable_id) - readable = can?(current_user, noteable_read_ability_name(noteable), noteable) - - return not_found!(noteables_str) unless readable + noteable = NotesFinder.new(user_project, current_user, params).target + noteable = nil unless can?(current_user, noteable_read_ability_name(noteable), noteable) + return not_found!(noteable_type) unless noteable noteable end + def params_by_noteable_type_and_id(type, id) + target_type = type.name.underscore + { target_type: target_type }.tap do |h| + if %w(issue merge_request).include?(target_type) + h[:target_iid] = id + else + h[:target_id] = id + end + end + end + def noteable_parent(noteable) public_send("user_#{noteable.class.parent_class.to_s.underscore}") # rubocop:disable GitlabSecurity/PublicSend end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 416cf39d3ec..9381f045144 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -15,8 +15,6 @@ module API requires :id, type: String, desc: "The ID of a #{parent_type}" end resource parent_type.pluralize.to_sym, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do - noteables_str = noteable_type.to_s.underscore.pluralize - desc "Get a list of #{noteable_type.to_s.downcase} notes" do success Entities::Note end @@ -30,7 +28,7 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ":id/#{noteables_str}/:noteable_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) # We exclude notes that are cross-references and that cannot be viewed # by the current user. By doing this exclusion at this level and not @@ -56,7 +54,7 @@ module API requires :noteable_id, type: Integer, desc: 'The ID of the noteable' end get ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) get_note(noteable, params[:note_id]) end @@ -69,7 +67,7 @@ module API optional :created_at, type: String, desc: 'The creation date of the note' end post ":id/#{noteables_str}/:noteable_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) opts = { note: params[:body], @@ -96,7 +94,7 @@ module API requires :body, type: String, desc: 'The content of a note' end put ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) update_note(noteable, params[:note_id]) end @@ -109,7 +107,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end delete ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) delete_note(noteable, params[:note_id]) end diff --git a/lib/api/resource_label_events.rb b/lib/api/resource_label_events.rb index 448bef12cec..505a6c68c9c 100644 --- a/lib/api/resource_label_events.rb +++ b/lib/api/resource_label_events.rb @@ -26,7 +26,7 @@ module API # rubocop: disable CodeReuse/ActiveRecord get ":id/#{eventables_str}/:eventable_id/resource_label_events" do - eventable = find_noteable(parent_type, eventables_str, params[:eventable_id]) + eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id]) events = eventable.resource_label_events.includes(:label, :user) present paginate(events), with: Entities::ResourceLabelEvent @@ -42,7 +42,7 @@ module API requires :eventable_id, types: [Integer, String], desc: 'The ID of the eventable' end get ":id/#{eventables_str}/:eventable_id/resource_label_events/:event_id" do - eventable = find_noteable(parent_type, eventables_str, params[:eventable_id]) + eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id]) event = eventable.resource_label_events.find(params[:event_id]) present event, with: Entities::ResourceLabelEvent diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 0a685152cf9..87bde4ca2f6 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -292,5 +292,31 @@ describe NotesFinder do expect(subject.target).to eq(commit) end end + + context 'target_iid' do + let(:issue) { create(:issue, project: project) } + 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) + 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) + 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 + 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) + end + end end end |