Merge branch 'osw-unnappliable-suggestion-on-expanded-lines' into 'master'

Adjust unnapliable suggestions in expanded lines

See merge request gitlab-org/gitlab!17286

app/models/diff_note.rb

@@ -75,6 +75,10 @@ class DiffNote < Note
     self.original_position.diff_refs == diff_refs
   end
 
+  # Checks if the current `position` line in the diff
+  # exists and is suggestible (not a deletion).
+  #
+  # Avoid using in iterations as it requests Gitaly.
   def supports_suggestion?
     return false unless noteable&.supports_suggestion? && on_text?
     # We don't want to trigger side-effects of `diff_file` call.

app/models/suggestion.rb

@@ -41,7 +41,6 @@ class Suggestion < ApplicationRecord
     !applied? &&
       noteable.opened? &&
       !outdated?(cached: cached) &&
-      note.supports_suggestion? &&
       different_content? &&
       note.active?
   end

changelogs/unreleased/osw-unnappliable-suggestion-on-expanded-lines.yml

+---
+title: Adjust unnapliable suggestions in expanded lines
+merge_request: 17286
+author:
+type: fixed

lib/gitlab/diff/position.rb

@@ -118,8 +118,14 @@ module Gitlab
             path: file_path
           }
 
+          # Takes action when creating diff notes (multiple calls are
+          # submitted to this method).
           Gitlab::SafeRequestStore.fetch(key) { find_diff_file(repository) }
         end
+
+        # We need to unfold diff lines according to the position in order
+        # to correctly calculate the line code and trace position changes.
+        @diff_file&.tap { |file| file.unfold_diff_lines(self) }
       end
 
       def diff_options
@@ -152,13 +158,7 @@ module Gitlab
         return unless diff_refs.complete?
         return unless comparison = diff_refs.compare_in(repository.project)
 
-        file = comparison.diffs(diff_options).diff_files.first
-
-        # We need to unfold diff lines according to the position in order
-        # to correctly calculate the line code and trace position changes.
-        file&.unfold_diff_lines(self)
-
-        file
+        comparison.diffs(diff_options).diff_files.first
       end
 
       def get_formatter_class(type)

spec/features/merge_request/user_suggests_changes_on_diff_spec.rb

@@ -14,6 +14,10 @@ describe 'User comments on a diff', :js do
     expect(suggested_content).to eq(expected_suggested_content)
   end
 
+  def expect_appliable_suggestions(amount)
+    expect(all('button', text: 'Apply suggestion').size).to eq(amount)
+  end
+
   let(:project) { create(:project, :repository) }
   let(:merge_request) do
     create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test')
@@ -89,6 +93,60 @@ describe 'User comments on a diff', :js do
     end
   end
 
+  context 'multiple suggestions in expanded lines' do
+    it 'suggestions are appliable' do
+      diff_file = merge_request.diffs(paths: ['files/ruby/popen.rb']).diff_files.first
+      hash = Digest::SHA1.hexdigest(diff_file.file_path)
+
+      expanded_changes = [
+        {
+          line_code: "#{hash}_1_1",
+          file_path: diff_file.file_path
+        },
+        {
+          line_code: "#{hash}_5_5",
+          file_path: diff_file.file_path
+        }
+      ]
+      changes = sample_compare(expanded_changes).changes.last(expanded_changes.size)
+
+      page.within("[id='#{hash}']") do
+        find("button[data-original-title='Show full file']").click
+        wait_for_requests
+
+        click_diff_line(find("[id='#{changes.first[:line_code]}']"))
+
+        page.within('.js-discussion-note-form') do
+          fill_in('note_note', with: "```suggestion\n# change to a comment\n```")
+          click_button('Comment')
+          wait_for_requests
+        end
+
+        click_diff_line(find("[id='#{changes.last[:line_code]}']"))
+
+        page.within('.js-discussion-note-form') do
+          fill_in('note_note', with: "```suggestion\n# 2nd change to a comment\n```")
+          click_button('Comment')
+          wait_for_requests
+        end
+
+        expect_appliable_suggestions(2)
+      end
+
+      # Making sure it's not a Front-end cache.
+      visit(diffs_project_merge_request_path(project, merge_request))
+
+      expect_appliable_suggestions(2)
+
+      page.within("[id='#{hash}']") do
+        all('button', text: 'Apply suggestion').last.click
+        wait_for_requests
+
+        expect(page).to have_content('Applied')
+      end
+    end
+  end
+
   context 'multiple suggestions in a single note' do
     it 'suggestions are presented' do
       click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))

spec/lib/gitlab/diff/position_spec.rb

@@ -130,6 +130,26 @@ describe Gitlab::Diff::Position do
           expect(diff_file.new_path).to eq(subject.new_path)
           expect(diff_file.diff_refs).to eq(subject.diff_refs)
         end
+
+        context 'different folded positions in the same diff file' do
+          def diff_file(args = {})
+            described_class
+              .new(args_for_text.merge(args))
+              .diff_file(project.repository)
+          end
+
+          it 'expands the diff file', :request_store do
+            expect_any_instance_of(Gitlab::Diff::File)
+              .to receive(:unfold_diff_lines).and_call_original
+
+            diff_file(old_line: 1, new_line: 1, diff_refs: commit.diff_refs)
+
+            expect_any_instance_of(Gitlab::Diff::File)
+              .to receive(:unfold_diff_lines).and_call_original
+
+            diff_file(old_line: 5, new_line: 5, diff_refs: commit.diff_refs)
+          end
+        end
       end
 
       describe "#diff_line" do

spec/models/suggestion_spec.rb

@@ -38,16 +38,6 @@ describe Suggestion do
   end
 
   describe '#appliable?' do
-    context 'when note does not support suggestions' do
-      it 'returns false' do
-        expect_next_instance_of(DiffNote) do |note|
-          allow(note).to receive(:supports_suggestion?) { false }
-        end
-
-        expect(suggestion).not_to be_appliable
-      end
-    end
-
     context 'when patch is already applied' do
       let(:suggestion) { create(:suggestion, :applied) }
 

spec/support/helpers/repo_helpers.rb

@@ -92,7 +92,7 @@ eos
     )
   end
 
-  def sample_compare
+  def sample_compare(extra_changes = [])
     changes = [
       {
         line_code: 'a5cc2925ca8258af241be7e5b0381edf30266302_20_20',
@@ -102,7 +102,7 @@ eos
         line_code: '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_4_6',
         file_path: '.gitmodules'
       }
-    ]
+    ] + extra_changes
 
     commits = %w(
       5937ac0a7beb003549fc5fd26fc247adbce4a52e