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

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

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

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'

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

changelogs/unreleased/fix-replying-to-commit-comment-in-mr-from-fork.yml

+---
+title: Fix replying to commit comments on merge requests created from forks
+merge_request:
+author:

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

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)