Suggestions: use template from target project instead of source project

Suggestions defined in MR would show values from target project when
previewing the commit message. However, once the commit was actually
created, it used values from source project instead. This means that
forks would ignore target project settings. The difference in preview
and result would also confuse users.
This MR changes it, so that suggestions always use target project when
generating the message, both for template definition and project name
and path variables.

Changelog: fixed

app/models/suggestion.rb

@@ -16,10 +16,14 @@ class Suggestion < ApplicationRecord
     note.latest_diff_file
   end
 
-  def project
+  def source_project
     noteable.source_project
   end
 
+  def target_project
+    noteable.target_project
+  end
+
   def branch
     noteable.source_branch
   end

app/policies/suggestion_policy.rb

 # frozen_string_literal: true
 
 class SuggestionPolicy < BasePolicy
-  delegate { @subject.project }
+  delegate { @subject.source_project }
 
   condition(:can_push_to_branch) do
-    Gitlab::UserAccess.new(@user, container: @subject.project).can_push_to_branch?(@subject.branch)
+    Gitlab::UserAccess.new(@user, container: @subject.source_project).can_push_to_branch?(@subject.branch)
   end
 
   rule { can_push_to_branch }.enable :apply_suggestion

app/services/suggestions/apply_service.rb

@@ -54,7 +54,7 @@ module Suggestions
         author_email: author&.email
       }
 
-      ::Files::MultiService.new(suggestion_set.project, current_user, params)
+      ::Files::MultiService.new(suggestion_set.source_project, current_user, params)
     end
 
     def commit_message

doc/user/project/merge_requests/reviews/suggestions.md

@@ -108,6 +108,8 @@ For example, to customize the commit message to output
 **Addresses user_1's review**, set the custom text to
 `Addresses %{username}'s review`.
 
+For merge requests created from forks, GitLab uses the template defined in target project.
+
 NOTE:
 Custom commit messages for each applied suggestion is
 introduced by [#25381](https://gitlab.com/gitlab-org/gitlab/-/issues/25381).

lib/gitlab/suggestions/commit_message.rb

@@ -13,7 +13,7 @@ module Gitlab
       end
 
       def message
-        project = suggestion_set.project
+        project = suggestion_set.target_project
         user_defined_message = @custom_message.presence || project.suggestion_commit_message.presence
         message = user_defined_message || DEFAULT_SUGGESTION_COMMIT_MESSAGE
 
@@ -37,8 +37,8 @@ module Gitlab
         'branch_name' => ->(user, suggestion_set) { suggestion_set.branch },
         'files_count' => ->(user, suggestion_set) { suggestion_set.file_paths.length },
         'file_paths' => ->(user, suggestion_set) { format_paths(suggestion_set.file_paths) },
-        'project_name' => ->(user, suggestion_set) { suggestion_set.project.name },
-        'project_path' => ->(user, suggestion_set) { suggestion_set.project.path },
+        'project_name' => ->(user, suggestion_set) { suggestion_set.target_project.name },
+        'project_path' => ->(user, suggestion_set) { suggestion_set.target_project.path },
         'user_full_name' => ->(user, suggestion_set) { user.name },
         'username' => ->(user, suggestion_set) { user.username },
         'suggestions_count' => ->(user, suggestion_set) { suggestion_set.suggestions.size }

lib/gitlab/suggestions/suggestion_set.rb

@@ -9,8 +9,12 @@ module Gitlab
         @suggestions = suggestions
       end
 
-      def project
-        first_suggestion.project
+      def source_project
+        first_suggestion.source_project
+      end
+
+      def target_project
+        first_suggestion.target_project
       end
 
       def branch

spec/lib/gitlab/suggestions/commit_message_spec.rb

@@ -3,7 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Suggestions::CommitMessage do
-  def create_suggestion(file_path, new_line, to_content)
+  include ProjectForksHelper
+  using RSpec::Parameterized::TableSyntax
+
+  def create_suggestion(merge_request, file_path, new_line, to_content)
     position = Gitlab::Diff::Position.new(old_path: file_path,
                                           new_path: file_path,
                                           old_line: nil,
@@ -29,69 +32,111 @@ RSpec.describe Gitlab::Suggestions::CommitMessage do
     create(:project, :repository, path: 'project-1', name: 'Project_1')
   end
 
-  let_it_be(:merge_request) do
+  let_it_be(:forked_project) { fork_project(project, nil, repository: true) }
+
+  let_it_be(:merge_request_same_project) do
     create(:merge_request, source_project: project, target_project: project)
   end
 
-  let_it_be(:suggestion_set) do
-    suggestion1 = create_suggestion('files/ruby/popen.rb', 9, '*** SUGGESTION 1 ***')
-    suggestion2 = create_suggestion('files/ruby/popen.rb', 13, '*** SUGGESTION 2 ***')
-    suggestion3 = create_suggestion('files/ruby/regex.rb', 22, '*** SUGGESTION 3 ***')
+  let_it_be(:merge_request_from_fork) do
+    create(:merge_request, source_project: forked_project, target_project: project)
+  end
+
+  let_it_be(:suggestion_set_same_project) do
+    suggestion1 = create_suggestion(merge_request_same_project, 'files/ruby/popen.rb', 9, '*** SUGGESTION 1 ***')
+    suggestion2 = create_suggestion(merge_request_same_project, 'files/ruby/popen.rb', 13, '*** SUGGESTION 2 ***')
+    suggestion3 = create_suggestion(merge_request_same_project, 'files/ruby/regex.rb', 22, '*** SUGGESTION 3 ***')
+
+    Gitlab::Suggestions::SuggestionSet.new([suggestion1, suggestion2, suggestion3])
+  end
+
+  let_it_be(:suggestion_set_forked_project) do
+    suggestion1 = create_suggestion(merge_request_from_fork, 'files/ruby/popen.rb', 9, '*** SUGGESTION 1 ***')
+    suggestion2 = create_suggestion(merge_request_from_fork, 'files/ruby/popen.rb', 13, '*** SUGGESTION 2 ***')
+    suggestion3 = create_suggestion(merge_request_from_fork, 'files/ruby/regex.rb', 22, '*** SUGGESTION 3 ***')
 
     Gitlab::Suggestions::SuggestionSet.new([suggestion1, suggestion2, suggestion3])
   end
 
   describe '#message' do
-    before do
-      # Updating the suggestion_commit_message on a project shared across specs
-      # avoids recreating the repository for each spec.
-      project.update!(suggestion_commit_message: message)
-    end
+    where(:suggestion_set) { [ref(:suggestion_set_same_project), ref(:suggestion_set_forked_project)] }
+
+    with_them do
+      before do
+        # Updating the suggestion_commit_message on a project shared across specs
+        # avoids recreating the repository for each spec.
+        project.update!(suggestion_commit_message: message)
+        forked_project.update!(suggestion_commit_message: fork_message)
+      end
+
+      let(:fork_message) { nil }
 
-    context 'when a custom commit message is not specified' do
-      let(:expected_message) { 'Apply 3 suggestion(s) to 2 file(s)' }
+      context 'when a custom commit message is not specified' do
+        let(:expected_message) { 'Apply 3 suggestion(s) to 2 file(s)' }
 
-      context 'and is nil' do
-        let(:message) { nil }
+        context 'and is nil' do
+          let(:message) { nil }
 
-        it 'uses the default commit message' do
-          expect(described_class
-                   .new(user, suggestion_set)
-                   .message).to eq(expected_message)
+          it 'uses the default commit message' do
+            expect(described_class
+                    .new(user, suggestion_set)
+                    .message).to eq(expected_message)
+          end
         end
-      end
 
-      context 'and is an empty string' do
-        let(:message) { '' }
+        context 'and is an empty string' do
+          let(:message) { '' }
 
-        it 'uses the default commit message' do
-          expect(described_class
-                   .new(user, suggestion_set)
-                   .message).to eq(expected_message)
+          it 'uses the default commit message' do
+            expect(described_class
+                    .new(user, suggestion_set)
+                    .message).to eq(expected_message)
+          end
         end
-      end
-    end
 
-    context 'when a custom commit message is specified' do
-      let(:message) { "i'm a project message. a user's custom message takes precedence over me :(" }
-      let(:custom_message) { "hello there! i'm a cool custom commit message." }
+        context 'when a custom commit message is specified for forked project' do
+          let(:message) { nil }
+          let(:fork_message) { "I'm a sad message that will not be used :(" }
 
-      it 'shows the custom commit message' do
-        expect(Gitlab::Suggestions::CommitMessage
-                 .new(user, suggestion_set, custom_message)
-                 .message).to eq(custom_message)
+          it 'uses the default commit message' do
+            expect(described_class
+                     .new(user, suggestion_set)
+                     .message).to eq(expected_message)
+          end
+        end
       end
-    end
 
-    context 'is specified and includes all placeholders' do
-      let(:message) do
-        '*** %{branch_name} %{files_count} %{file_paths} %{project_name} %{project_path} %{user_full_name} %{username} %{suggestions_count} ***'
+      context 'when a custom commit message is specified' do
+        let(:message) { "i'm a project message. a user's custom message takes precedence over me :(" }
+        let(:custom_message) { "hello there! i'm a cool custom commit message." }
+
+        it 'shows the custom commit message' do
+          expect(Gitlab::Suggestions::CommitMessage
+                  .new(user, suggestion_set, custom_message)
+                  .message).to eq(custom_message)
+        end
       end
 
-      it 'generates a custom commit message' do
-        expect(Gitlab::Suggestions::CommitMessage
-                 .new(user, suggestion_set)
-                 .message).to eq('*** master 2 files/ruby/popen.rb, files/ruby/regex.rb Project_1 project-1 Test User test.user 3 ***')
+      context 'is specified and includes all placeholders' do
+        let(:message) do
+          '*** %{branch_name} %{files_count} %{file_paths} %{project_name} %{project_path} %{user_full_name} %{username} %{suggestions_count} ***'
+        end
+
+        it 'generates a custom commit message' do
+          expect(Gitlab::Suggestions::CommitMessage
+                  .new(user, suggestion_set)
+                  .message).to eq('*** master 2 files/ruby/popen.rb, files/ruby/regex.rb Project_1 project-1 Test User test.user 3 ***')
+        end
+
+        context 'when a custom commit message is specified for forked project' do
+          let(:fork_message) { "I'm a sad message that will not be used :(" }
+
+          it 'uses the target project commit message' do
+            expect(Gitlab::Suggestions::CommitMessage
+                    .new(user, suggestion_set)
+                    .message).to eq('*** master 2 files/ruby/popen.rb, files/ruby/regex.rb Project_1 project-1 Test User test.user 3 ***')
+          end
+        end
       end
     end
   end

spec/lib/gitlab/suggestions/suggestion_set_spec.rb

@@ -3,6 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Suggestions::SuggestionSet do
+  include ProjectForksHelper
+  using RSpec::Parameterized::TableSyntax
+
   def create_suggestion(file_path, new_line, to_content)
     position = Gitlab::Diff::Position.new(old_path: file_path,
                                           new_path: file_path,
@@ -24,86 +27,99 @@ RSpec.describe Gitlab::Suggestions::SuggestionSet do
   let_it_be(:user) { create(:user) }
 
   let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:forked_project) { fork_project(project, nil, repository: true) }
 
-  let_it_be(:merge_request) do
+  let_it_be(:merge_request_same_project) do
     create(:merge_request, source_project: project, target_project: project)
   end
 
-  let_it_be(:suggestion) { create(:suggestion)}
-
-  let_it_be(:suggestion2) do
-    create_suggestion('files/ruby/popen.rb', 13, "*** SUGGESTION 2 ***")
-  end
-
-  let_it_be(:suggestion3) do
-    create_suggestion('files/ruby/regex.rb', 22, "*** SUGGESTION 3 ***")
+  let_it_be(:merge_request_from_fork) do
+    create(:merge_request, source_project: forked_project, target_project: project)
   end
 
-  let_it_be(:unappliable_suggestion) { create(:suggestion, :unappliable) }
+  where(:merge_request) { [ref(:merge_request_same_project), ref(:merge_request_from_fork)] }
+  with_them do
+    let(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) }
+    let(:suggestion) { create(:suggestion, note: note) }
 
-  let(:suggestion_set) { described_class.new([suggestion]) }
-
-  describe '#project' do
-    it 'returns the project associated with the suggestions' do
-      expected_project = suggestion.project
+    let(:suggestion2) do
+      create_suggestion('files/ruby/popen.rb', 13, "*** SUGGESTION 2 ***")
+    end
 
-      expect(suggestion_set.project).to be(expected_project)
+    let(:suggestion3) do
+      create_suggestion('files/ruby/regex.rb', 22, "*** SUGGESTION 3 ***")
     end
-  end
 
-  describe '#branch' do
-    it 'returns the branch associated with the suggestions' do
-      expected_branch = suggestion.branch
+    let(:unappliable_suggestion) { create(:suggestion, :unappliable) }
+
+    let(:suggestion_set) { described_class.new([suggestion]) }
 
-      expect(suggestion_set.branch).to be(expected_branch)
+    describe '#source_project' do
+      it 'returns the source project associated with the suggestions' do
+        expect(suggestion_set.source_project).to be(merge_request.source_project)
+      end
     end
-  end
 
-  describe '#valid?' do
-    it 'returns true if no errors are found' do
-      expect(suggestion_set.valid?).to be(true)
+    describe '#target_project' do
+      it 'returns the target project associated with the suggestions' do
+        expect(suggestion_set.target_project).to be(project)
+      end
     end
 
-    it 'returns false if an error is found' do
-      suggestion_set = described_class.new([unappliable_suggestion])
+    describe '#branch' do
+      it 'returns the branch associated with the suggestions' do
+        expected_branch = suggestion.branch
 
-      expect(suggestion_set.valid?).to be(false)
+        expect(suggestion_set.branch).to be(expected_branch)
+      end
     end
-  end
 
-  describe '#error_message' do
-    it 'returns an error message if an error is found' do
-      suggestion_set = described_class.new([unappliable_suggestion])
+    describe '#valid?' do
+      it 'returns true if no errors are found' do
+        expect(suggestion_set.valid?).to be(true)
+      end
 
-      expect(suggestion_set.error_message).to be_a(String)
+      it 'returns false if an error is found' do
+        suggestion_set = described_class.new([unappliable_suggestion])
+
+        expect(suggestion_set.valid?).to be(false)
+      end
     end
 
-    it 'returns nil if no errors are found' do
-      expect(suggestion_set.error_message).to be(nil)
+    describe '#error_message' do
+      it 'returns an error message if an error is found' do
+        suggestion_set = described_class.new([unappliable_suggestion])
+
+        expect(suggestion_set.error_message).to be_a(String)
+      end
+
+      it 'returns nil if no errors are found' do
+        expect(suggestion_set.error_message).to be(nil)
+      end
     end
-  end
 
-  describe '#actions' do
-    it 'returns an array of hashes with proper key/value pairs' do
-      first_action = suggestion_set.actions.first
+    describe '#actions' do
+      it 'returns an array of hashes with proper key/value pairs' do
+        first_action = suggestion_set.actions.first
 
-      file_suggestion = suggestion_set.send(:suggestions_per_file).first
+        file_suggestion = suggestion_set.send(:suggestions_per_file).first
 
-      expect(first_action[:action]).to be('update')
-      expect(first_action[:file_path]).to eq(file_suggestion.file_path)
-      expect(first_action[:content]).to eq(file_suggestion.new_content)
+        expect(first_action[:action]).to be('update')
+        expect(first_action[:file_path]).to eq(file_suggestion.file_path)
+        expect(first_action[:content]).to eq(file_suggestion.new_content)
+      end
     end
-  end
 
-  describe '#file_paths' do
-    it 'returns an array of unique file paths associated with the suggestions' do
-      suggestion_set = described_class.new([suggestion, suggestion2, suggestion3])
+    describe '#file_paths' do
+      it 'returns an array of unique file paths associated with the suggestions' do
+        suggestion_set = described_class.new([suggestion, suggestion2, suggestion3])
 
-      expected_paths = %w(files/ruby/popen.rb files/ruby/regex.rb)
+        expected_paths = %w(files/ruby/popen.rb files/ruby/regex.rb)
 
-      actual_paths = suggestion_set.file_paths
+        actual_paths = suggestion_set.file_paths
 
-      expect(actual_paths.sort).to eq(expected_paths)
+        expect(actual_paths.sort).to eq(expected_paths)
+      end
     end
   end
 end