kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Use NotesFinder to fetch notes on API and Controllers

Browse source 
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
This commit is contained in:
Patrick Derichs 2019-06-15 07:10:58 +02:00
parent ba952d53c5
commit 932a9a0c77
7 changed files with 71 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
app/finders/notes_finder.rb
View file

@ -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

18
lib/api/discussions.rb
View file

@ -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

5
lib/api/helpers.rb
View file

@ -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)

21
lib/api/helpers/notes_helpers.rb
View file

@ -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

12
lib/api/notes.rb
View file

@ -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

4
lib/api/resource_label_events.rb
View file

@ -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

26
spec/finders/notes_finder_spec.rb
View file

@ -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