From 75d04f6a29a5506ffd53c227517411febdc54910 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 27 Jul 2017 15:36:39 +0100 Subject: Fix replying to commit comments on MRs from forks A commit comment shows in the MR, but if the MR is from a fork, it will have a different project ID to the MR's target project. In that case, add an note_project_id param so that we can pick the correct project for the note. --- app/assets/javascripts/notes.js | 4 ++ app/controllers/concerns/notes_actions.rb | 22 +++++++- app/helpers/notes_helper.rb | 6 ++- app/views/shared/notes/_form.html.haml | 1 + ...-replying-to-commit-comment-in-mr-from-fork.yml | 4 ++ spec/controllers/projects/notes_controller_spec.rb | 62 +++++++++++++++++++++- .../merge_requests/created_from_fork_spec.rb | 27 ++++++++++ 7 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/fix-replying-to-commit-comment-in-mr-from-fork.yml diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index b2c503d1656..dfa07a2def4 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -529,6 +529,7 @@ export default class Notes { form.find('#note_line_code').remove(); form.find('#note_position').remove(); form.find('#note_type').val(''); + form.find('#note_project_id').remove(); form.find('#in_reply_to_discussion_id').remove(); form.find('.js-comment-resolve-button').closest('comment-and-resolve-btn').remove(); this.parentTimeline = form.parents('.timeline'); @@ -556,6 +557,7 @@ export default class Notes { form.find('#note_noteable_id').val(), form.find('#note_commit_id').val(), form.find('#note_type').val(), + form.find('#note_project_id').val(), form.find('#in_reply_to_discussion_id').val(), // LegacyDiffNote @@ -848,6 +850,8 @@ export default class Notes { form.find('#in_reply_to_discussion_id').val(discussionID); } + form.find('#note_project_id').val(dataHolder.data('discussionProjectId')); + form.attr('data-line-code', dataHolder.data('lineCode')); form.find('#line_type').val(dataHolder.data('lineType')); diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index a57d9e6e6c0..af5f683bab5 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -4,6 +4,7 @@ module NotesActions included do before_action :authorize_admin_note!, only: [:update, :destroy] + before_action :note_project, only: [:create] end def index @@ -28,7 +29,8 @@ module NotesActions 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(project, current_user, create_params).execute + + @note = Notes::CreateService.new(note_project, current_user, create_params).execute if @note.is_a?(Note) Banzai::NoteRenderer.render([@note], @project, current_user) @@ -177,4 +179,22 @@ module NotesActions def notes_finder @notes_finder ||= NotesFinder.new(project, current_user, finder_params) end + + def note_project + return @note_project if defined?(@note_project) + return nil unless project + + note_project_id = params[:note_project_id] + + @note_project = + if note_project_id.present? + Project.find(note_project_id) + else + project + end + + return access_denied! unless can?(current_user, :create_note, @note_project) + + @note_project + end end diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 0a0881d95cf..8f4e39b8b23 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -62,7 +62,11 @@ module NotesHelper def link_to_reply_discussion(discussion, line_type = nil) return unless current_user - data = { discussion_id: discussion.reply_id, line_type: line_type } + data = { + discussion_id: discussion.reply_id, + discussion_project_id: discussion.project&.id, + line_type: line_type + } button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button', data: data, title: 'Add a reply' diff --git a/app/views/shared/notes/_form.html.haml b/app/views/shared/notes/_form.html.haml index c6b5dcc3647..725bf916592 100644 --- a/app/views/shared/notes/_form.html.haml +++ b/app/views/shared/notes/_form.html.haml @@ -10,6 +10,7 @@ = hidden_field_tag :line_type = hidden_field_tag :merge_request_diff_head_sha, @note.noteable.try(:diff_head_sha) = hidden_field_tag :in_reply_to_discussion_id + = hidden_field_tag :note_project_id = note_target_fields(@note) = f.hidden_field :noteable_type diff --git a/changelogs/unreleased/fix-replying-to-commit-comment-in-mr-from-fork.yml b/changelogs/unreleased/fix-replying-to-commit-comment-in-mr-from-fork.yml new file mode 100644 index 00000000000..f4136460626 --- /dev/null +++ b/changelogs/unreleased/fix-replying-to-commit-comment-in-mr-from-fork.yml @@ -0,0 +1,4 @@ +--- +title: Fix replying to commit comments on merge requests created from forks +merge_request: +author: diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 45f4cf9180d..3b88d5b0d7d 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -131,7 +131,7 @@ describe Projects::NotesController do before do sign_in(user) - project.team << [user, :developer] + project.add_developer(user) end it "returns status 302 for html" do @@ -165,6 +165,66 @@ describe Projects::NotesController do expect(response).to have_http_status(302) end end + + context 'when creating a commit comment from an MR fork' do + let(:project) { create(:project) } + + let(:fork_project) do + create(:project).tap do |fork| + create(:forked_project_link, forked_to_project: fork, forked_from_project: project) + end + end + + let(:merge_request) do + create(:merge_request, source_project: fork_project, target_project: project, source_branch: 'feature', target_branch: 'master') + end + + let(:existing_comment) do + create(:note_on_commit, note: 'a note', project: fork_project, commit_id: merge_request.commit_shas.first) + end + + def post_create(extra_params = {}) + post :create, { + note: { note: 'some other note' }, + namespace_id: project.namespace, + project_id: project, + target_type: 'merge_request', + target_id: merge_request.id, + note_project_id: fork_project.id, + in_reply_to_discussion_id: existing_comment.discussion_id + }.merge(extra_params) + end + + context 'when the note_project_id is not correct' do + it 'returns a 404' do + post_create(note_project_id: Project.maximum(:id).succ) + + expect(response).to have_http_status(404) + end + end + + context 'when the user has no access to the fork' do + it 'returns a 404' do + post_create + + expect(response).to have_http_status(404) + end + end + + context 'when the user has access to the fork' do + let(:discussion) { fork_project.notes.find_discussion(existing_comment.discussion_id) } + + before do + fork_project.add_developer(user) + + existing_comment + end + + it 'creates the note' do + expect { post_create }.to change { fork_project.notes.count }.by(1) + end + end + end end describe 'DELETE destroy' do diff --git a/spec/features/merge_requests/created_from_fork_spec.rb b/spec/features/merge_requests/created_from_fork_spec.rb index 9b7795ace62..d706d01dde0 100644 --- a/spec/features/merge_requests/created_from_fork_spec.rb +++ b/spec/features/merge_requests/created_from_fork_spec.rb @@ -25,6 +25,33 @@ feature 'Merge request created from fork' do expect(page).to have_content 'Test merge request' end + context 'when a commit comment exists on the merge request' do + given(:comment) { 'A commit comment' } + given(:reply) { 'A reply comment' } + + background do + create(:note_on_commit, note: comment, + project: fork_project, + commit_id: merge_request.commit_shas.first) + end + + scenario 'user can reply to the comment', js: true do + visit_merge_request(merge_request) + + expect(page).to have_content(comment) + + page.within('.discussion-notes') do + find('.btn-text-field').click + find('#note_note').send_keys(reply) + find('.comment-btn').click + end + + wait_for_requests + + expect(page).to have_content(reply) + end + end + context 'source project is deleted' do background do MergeRequests::MergeService.new(project, user).execute(merge_request) -- cgit v1.2.1