Merge branch 'suggestions-use-target-instead-of-source-project' into 'master'

Suggestions: use template from target project instead of source project

See merge request gitlab-org/gitlab!82657

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,25 +32,45 @@ 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
+    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)' }
 
@@ -70,6 +93,17 @@ RSpec.describe Gitlab::Suggestions::CommitMessage do
                     .message).to eq(expected_message)
           end
         end
+
+        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 'uses the default commit message' do
+            expect(described_class
+                     .new(user, suggestion_set)
+                     .message).to eq(expected_message)
+          end
+        end
       end
 
       context 'when a custom commit message is specified' do
@@ -93,6 +127,17 @@ RSpec.describe Gitlab::Suggestions::CommitMessage do
                   .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
 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,30 +27,42 @@ 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(:merge_request_from_fork) do
+    create(:merge_request, source_project: forked_project, target_project: project)
+  end
+
+  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_it_be(:suggestion2) do
+    let(:suggestion2) do
       create_suggestion('files/ruby/popen.rb', 13, "*** SUGGESTION 2 ***")
     end
 
-  let_it_be(:suggestion3) do
+    let(:suggestion3) do
       create_suggestion('files/ruby/regex.rb', 22, "*** SUGGESTION 3 ***")
     end
 
-  let_it_be(:unappliable_suggestion) { create(:suggestion, :unappliable) }
+    let(:unappliable_suggestion) { create(:suggestion, :unappliable) }
 
     let(:suggestion_set) { described_class.new([suggestion]) }
 
-  describe '#project' do
-    it 'returns the project associated with the suggestions' do
-      expected_project = suggestion.project
+    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
 
-      expect(suggestion_set.project).to be(expected_project)
+    describe '#target_project' do
+      it 'returns the target project associated with the suggestions' do
+        expect(suggestion_set.target_project).to be(project)
       end
     end
 
@@ -106,4 +121,5 @@ RSpec.describe Gitlab::Suggestions::SuggestionSet do
         expect(actual_paths.sort).to eq(expected_paths)
       end
     end
+  end
 end