Commit c44c83c4 authored by Lin Jen-Shin's avatar Lin Jen-Shin

Merge branch '54924-refactor-notes-actions-params' into 'master'

Refactor params for NotesActions

Closes #54924

See merge request gitlab-org/gitlab-ce!25278
parents 503e1ff1 d03dee26
...@@ -6,7 +6,6 @@ module NotesActions ...@@ -6,7 +6,6 @@ module NotesActions
extend ActiveSupport::Concern extend ActiveSupport::Concern
included do included do
prepend_before_action :normalize_create_params, only: [:create]
before_action :set_polling_interval_header, only: [:index] before_action :set_polling_interval_header, only: [:index]
before_action :require_noteable!, only: [:index, :create] before_action :require_noteable!, only: [:index, :create]
before_action :authorize_admin_note!, only: [:update, :destroy] before_action :authorize_admin_note!, only: [:update, :destroy]
...@@ -44,12 +43,7 @@ module NotesActions ...@@ -44,12 +43,7 @@ module NotesActions
# rubocop:disable Gitlab/ModuleWithInstanceVariables # rubocop:disable Gitlab/ModuleWithInstanceVariables
def create def create
create_params = note_params.merge( @note = Notes::CreateService.new(note_project, current_user, create_note_params).execute
merge_request_diff_head_sha: params[:merge_request_diff_head_sha],
in_reply_to_discussion_id: params[:in_reply_to_discussion_id]
)
@note = Notes::CreateService.new(note_project, current_user, create_params).execute
respond_to do |format| respond_to do |format|
format.json do format.json do
...@@ -78,7 +72,7 @@ module NotesActions ...@@ -78,7 +72,7 @@ module NotesActions
# rubocop:disable Gitlab/ModuleWithInstanceVariables # rubocop:disable Gitlab/ModuleWithInstanceVariables
def update def update
@note = Notes::UpdateService.new(project, current_user, note_params).execute(note) @note = Notes::UpdateService.new(project, current_user, update_note_params).execute(note)
prepare_notes_for_rendering([@note]) prepare_notes_for_rendering([@note])
respond_to do |format| respond_to do |format|
...@@ -196,24 +190,36 @@ module NotesActions ...@@ -196,24 +190,36 @@ module NotesActions
return access_denied! unless can?(current_user, :admin_note, note) return access_denied! unless can?(current_user, :admin_note, note)
end end
def note_params def create_note_params
params.require(:note).permit( params.require(:note).permit(
:project_id,
:noteable_type,
:noteable_id,
:commit_id,
:noteable,
:type, :type,
:note, :note,
:attachment, :line_code, # LegacyDiffNote
:position # DiffNote
).tap do |create_params|
create_params.merge!(
params.permit(:merge_request_diff_head_sha, :in_reply_to_discussion_id)
)
# LegacyDiffNote # These params are also sent by the client but we need to set these based on
:line_code, # target_type and target_id because we're checking permissions based on that
create_params[:noteable_type] = params[:target_type].classify
case params[:target_type]
when 'commit'
create_params[:commit_id] = params[:target_id]
when 'merge_request'
create_params[:noteable_id] = params[:target_id]
# Notes on MergeRequest can have an extra `commit_id` context
create_params[:commit_id] = params.dig(:note, :commit_id)
else
create_params[:noteable_id] = params[:target_id]
end
end
end
# DiffNote def update_note_params
:position params.require(:note).permit(:note)
)
end end
def set_polling_interval_header def set_polling_interval_header
...@@ -248,15 +254,6 @@ module NotesActions ...@@ -248,15 +254,6 @@ module NotesActions
DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: ProjectNoteEntity) DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: ProjectNoteEntity)
end end
# Avoids checking permissions in the wrong object - this ensures that the object we checked permissions for
# is the object we're actually creating a note in.
def normalize_create_params
params[:note].try do |note|
note[:noteable_id] = params[:target_id]
note[:noteable_type] = params[:target_type].classify
end
end
def note_project def note_project
strong_memoize(:note_project) do strong_memoize(:note_project) do
next nil unless project next nil unless project
......
...@@ -26,10 +26,6 @@ class Snippets::NotesController < ApplicationController ...@@ -26,10 +26,6 @@ class Snippets::NotesController < ApplicationController
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
alias_method :noteable, :snippet alias_method :noteable, :snippet
def note_params
super.merge(noteable_id: params[:snippet_id])
end
def finder_params 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')
end end
......
---
title: Fix commenting on commits having SHA1 starting with a large number
merge_request: 25278
author:
type: fixed
...@@ -252,8 +252,8 @@ describe Projects::NotesController do ...@@ -252,8 +252,8 @@ describe Projects::NotesController do
note: 'some note', note: 'some note',
noteable_id: merge_request.id.to_s, noteable_id: merge_request.id.to_s,
noteable_type: 'MergeRequest', noteable_type: 'MergeRequest',
merge_request_diff_head_sha: 'sha', commit_id: nil,
in_reply_to_discussion_id: nil merge_request_diff_head_sha: 'sha'
}).permit! }).permit!
expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true)) expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true))
...@@ -266,6 +266,22 @@ describe Projects::NotesController do ...@@ -266,6 +266,22 @@ describe Projects::NotesController do
end end
end end
context 'when creating a comment on a commit with SHA1 starting with a large number' do
let(:commit) { create(:commit, project: project, id: '842616594688d2351480dfebd67b3d8d15571e6d') }
it 'creates a note successfully' do
expect do
post :create, params: {
note: { note: 'some note', commit_id: commit.id },
namespace_id: project.namespace,
project_id: project,
target_type: 'commit',
target_id: commit.id
}
end.to change { Note.count }.by(1)
end
end
context 'when creating a commit comment from an MR fork' do context 'when creating a commit comment from an MR fork' do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
......
...@@ -63,7 +63,8 @@ module TestEnv ...@@ -63,7 +63,8 @@ module TestEnv
'after-create-delete-modify-move' => 'ba3faa7', 'after-create-delete-modify-move' => 'ba3faa7',
'with-codeowners' => '219560e', 'with-codeowners' => '219560e',
'submodule_inside_folder' => 'b491b92', 'submodule_inside_folder' => 'b491b92',
'png-lfs' => 'fe42f41' 'png-lfs' => 'fe42f41',
'sha-starting-with-large-number' => '8426165'
}.freeze }.freeze
# gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily
......
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