Fix replying to commit comments on MRs from forksfix-replying-to-commit-comment-in-mr-from-fork

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.

10 files changed, 164 insertions, 32 deletions

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..bfc9a273f92 100644
--- a/app/controllers/concerns/notes_actions.rb
+++ b/app/controllers/concerns/notes_actions.rb
@@ -26,7 +26,8 @@ module NotesActions
   def create
     create_params = note_params.merge(
       merge_request_diff_head_sha: params[:merge_request_diff_head_sha],
-      in_reply_to_discussion_id: params[:in_reply_to_discussion_id]
+      in_reply_to_discussion_id: params[:in_reply_to_discussion_id],
+      note_project_id: params[:note_project_id]
     )
     @note = Notes::CreateService.new(project, current_user, create_params).execute
 
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/services/notes/build_service.rb b/app/services/notes/build_service.rb
index abf25bb778b..8d34ee5e0a8 100644
--- a/app/services/notes/build_service.rb
+++ b/app/services/notes/build_service.rb
@@ -3,28 +3,38 @@ module Notes
     def execute
       in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
 
+      # Only used for discussions at present, as in other contexts you won't see
+      # a note from another project - only when an MR is created from a fork,
+      # and there are commit comments on the fork.
+      note_project_id = params.delete(:note_project_id)
+      note_project = project
+
       if in_reply_to_discussion_id.present?
-        discussion = find_discussion(in_reply_to_discussion_id)
+        if note_project_id.present?
+          note_project = Project.find_by(id: note_project_id)
 
-        unless discussion
-          note = Note.new
-          note.errors.add(:base, 'Discussion to reply to cannot be found')
-          return note
+          return error_note unless note_project
         end
 
+        discussion = find_discussion(in_reply_to_discussion_id, note_project)
+
+        return error_note unless discussion
+
         params.merge!(discussion.reply_attributes)
       end
 
       note = Note.new(params)
-      note.project = project
+      note.project = note_project
       note.author = current_user
 
       note
     end
 
-    def find_discussion(discussion_id)
-      if project
-        project.notes.find_discussion(discussion_id)
+    def find_discussion(discussion_id, discussion_project)
+      if discussion_project
+        return nil unless can?(current_user, :create_note, discussion_project)
+
+        discussion_project.notes.find_discussion(discussion_id)
       else
         # only PersonalSnippets can have discussions without project association
         discussion = Note.find_discussion(discussion_id)
@@ -35,5 +45,13 @@ module Notes
         discussion
       end
     end
+
+    private
+
+    def error_note
+      note = Note.new
+      note.errors.add(:base, 'Discussion to reply to cannot be found')
+      note
+    end
   end
 end
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..0c1ea717135 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -153,7 +153,8 @@ describe Projects::NotesController do
           noteable_id: merge_request.id.to_s,
           noteable_type: 'MergeRequest',
           merge_request_diff_head_sha: 'sha',
-          in_reply_to_discussion_id: nil
+          in_reply_to_discussion_id: nil,
+          note_project_id: nil
         }
 
         expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true))
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)
diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb
index 8b6b02916ae..08209ed2f26 100644
--- a/spec/models/sent_notification_spec.rb
+++ b/spec/models/sent_notification_spec.rb
@@ -1,6 +1,9 @@
 require 'spec_helper'
 
 describe SentNotification do
+  let(:issue_project) { create(:empty_project, :public) }
+  let(:mr_project) { create(:project, :public) }
+
   describe 'validation' do
     describe 'note validity' do
       context "when the project doesn't match the noteable's project" do
@@ -21,10 +24,9 @@ describe SentNotification do
       end
 
       context "when the noteable project and discussion project match" do
-        let(:project) { create(:project) }
-        let(:issue) { create(:issue, project: project) }
-        let(:discussion_id) { create(:note, project: project, noteable: issue).discussion_id }
-        subject { build(:sent_notification, project: project, noteable: issue, in_reply_to_discussion_id: discussion_id) }
+        let(:issue) { create(:issue, project: issue_project) }
+        let(:discussion_id) { create(:note, project: issue_project, noteable: issue).discussion_id }
+        subject { build(:sent_notification, project: issue_project, noteable: issue, in_reply_to_discussion_id: discussion_id) }
 
         it "is valid" do
           expect(subject).to be_valid
@@ -44,7 +46,7 @@ describe SentNotification do
 
   describe '.record_note' do
     let(:user) { create(:user) }
-    let(:note) { create(:diff_note_on_merge_request) }
+    let(:note) { create(:diff_note_on_merge_request, project: mr_project) }
 
     it 'creates a new SentNotification' do
       expect { described_class.record_note(note, user.id) }.to change { described_class.count }.by(1)
@@ -53,7 +55,7 @@ describe SentNotification do
 
   describe '#create_reply' do
     context 'for issue' do
-      let(:issue) { create(:issue) }
+      let(:issue) { create(:issue, project: issue_project) }
       subject { described_class.record(issue, issue.author.id) }
 
       it 'creates a comment on the issue' do
@@ -63,7 +65,7 @@ describe SentNotification do
     end
 
     context 'for issue comment' do
-      let(:note) { create(:note_on_issue) }
+      let(:note) { create(:note_on_issue, project: issue_project) }
       subject { described_class.record_note(note, note.author.id) }
 
       it 'creates a comment on the issue' do
@@ -74,7 +76,7 @@ describe SentNotification do
     end
 
     context 'for issue discussion' do
-      let(:note) { create(:discussion_note_on_issue) }
+      let(:note) { create(:discussion_note_on_issue, project: issue_project) }
       subject { described_class.record_note(note, note.author.id) }
 
       it 'creates a reply on the discussion' do
@@ -85,7 +87,7 @@ describe SentNotification do
     end
 
     context 'for merge request' do
-      let(:merge_request) { create(:merge_request) }
+      let(:merge_request) { create(:merge_request, source_project: mr_project) }
       subject { described_class.record(merge_request, merge_request.author.id) }
 
       it 'creates a comment on the merge_request' do
@@ -95,7 +97,7 @@ describe SentNotification do
     end
 
     context 'for merge request comment' do
-      let(:note) { create(:note_on_merge_request) }
+      let(:note) { create(:note_on_merge_request, project: mr_project) }
       subject { described_class.record_note(note, note.author.id) }
 
       it 'creates a comment on the merge request' do
@@ -106,7 +108,7 @@ describe SentNotification do
     end
 
     context 'for merge request diff discussion' do
-      let(:note) { create(:diff_note_on_merge_request) }
+      let(:note) { create(:diff_note_on_merge_request, project: mr_project) }
       subject { described_class.record_note(note, note.author.id) }
 
       it 'creates a reply on the discussion' do
@@ -117,7 +119,7 @@ describe SentNotification do
     end
 
     context 'for merge request non-diff discussion' do
-      let(:note) { create(:discussion_note_on_merge_request) }
+      let(:note) { create(:discussion_note_on_merge_request, project: mr_project) }
       subject { described_class.record_note(note, note.author.id) }
 
       it 'creates a reply on the discussion' do
@@ -139,7 +141,7 @@ describe SentNotification do
     end
 
     context 'for commit comment' do
-      let(:note) { create(:note_on_commit) }
+      let(:note) { create(:note_on_commit, project: mr_project) }
       subject { described_class.record_note(note, note.author.id) }
 
       it 'creates a comment on the commit' do
@@ -150,7 +152,7 @@ describe SentNotification do
     end
 
     context 'for commit diff discussion' do
-      let(:note) { create(:diff_note_on_commit) }
+      let(:note) { create(:diff_note_on_commit, project: mr_project) }
       subject { described_class.record_note(note, note.author.id) }
 
       it 'creates a reply on the discussion' do
@@ -161,7 +163,7 @@ describe SentNotification do
     end
 
     context 'for commit non-diff discussion' do
-      let(:note) { create(:discussion_note_on_commit) }
+      let(:note) { create(:discussion_note_on_commit, project: mr_project) }
       subject { described_class.record_note(note, note.author.id) }
 
       it 'creates a reply on the discussion' do
diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb
index 6e1c1fe6c02..0e36ddc0184 100644
--- a/spec/services/notes/build_service_spec.rb
+++ b/spec/services/notes/build_service_spec.rb
@@ -1,35 +1,105 @@
 require 'spec_helper'
 
 describe Notes::BuildService do
-  let(:note) { create(:discussion_note_on_issue) }
-  let(:project) { note.project }
+  let(:project) { create(:empty_project, :public) }
+  let(:note) { create(:discussion_note_on_issue, project: project) }
   let(:author) { note.author }
 
   describe '#execute' do
     context 'when in_reply_to_discussion_id is specified' do
       context 'when a note with that original discussion ID exists' do
+        let(:new_note) do
+          described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
+        end
+
         it 'sets the note up to be in reply to that note' do
-          new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
           expect(new_note).to be_valid
           expect(new_note.in_reply_to?(note)).to be_truthy
         end
       end
 
       context 'when a note with that discussion ID exists' do
+        let(:new_note) do
+          described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
+        end
+
         it 'sets the note up to be in reply to that note' do
-          new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
           expect(new_note).to be_valid
           expect(new_note.in_reply_to?(note)).to be_truthy
         end
       end
 
       context 'when no note with that discussion ID exists' do
+        let(:new_note) do
+          described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: 'foo').execute
+        end
+
         it 'sets an error' do
-          new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: 'foo').execute
           expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found')
         end
       end
 
+      context 'when note_project_id is also specified' do
+        let(:base_params) do
+          { note: 'Test', in_reply_to_discussion_id: note.discussion_id }
+        end
+
+        let(:upstream_project) do
+          create(:empty_project, :public).tap do |new_project|
+            create(:forked_project_link, forked_to_project: project, forked_from_project: new_project)
+          end
+        end
+
+        context 'when the note exists in the note_project_id' do
+          let(:new_note) do
+            described_class.new(upstream_project, author, base_params.merge(note_project_id: project.id)).execute
+          end
+
+          it 'sets the note up to be in reply to that note' do
+            expect(new_note).to be_valid
+            expect(new_note.in_reply_to?(note)).to be_truthy
+          end
+
+          it 'sets the new note on the correct project' do
+            expect(new_note.project).to eq(project)
+          end
+        end
+
+        context 'when the note exists in the original project' do
+          let(:new_note) do
+            described_class.new(project, author, base_params.merge(note_project_id: upstream_project.id)).execute
+          end
+
+          it 'sets an error' do
+            expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found')
+          end
+        end
+
+        context 'when the note_project_id does not exist' do
+          let(:new_note) do
+            described_class.new(upstream_project, author, base_params.merge(note_project_id: Project.maximum(:id).succ)).execute
+          end
+
+          it 'sets an error' do
+            expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found')
+          end
+        end
+
+        context 'when the user cannot add notes to the project the discussion belongs to' do
+          before do
+            project.update!(visibility_level: Project::PRIVATE)
+          end
+
+          let(:new_note) do
+            described_class.new(upstream_project, author, base_params.merge(note_project_id: project.id)).execute
+          end
+
+          it 'sets an error' do
+            expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found')
+          end
+        end
+      end
+
       context 'personal snippet note' do
         def reply(note, user = nil)
           user ||= create(:user)