Commit 7998e114 authored by Oswaldo Ferreira's avatar Oswaldo Ferreira Committed by Douglas Barbosa Alexandre

Adjust unnapliable suggestions in expanded lines

When expanding diff lines and leaving suggestion comments,
eventually, they showed as unnapliable
even with no changes in the diff.

It happened given the `DiffNote#supports_suggestion?` check was
being made at `Suggestion#appliable?` in the discussions iteration.

The problem with `DiffNote#supports_suggestion?` as it is that
it uses `Position#diff_file`, which has a RequestStore "cache".

So for instance, if we have 3 comments in expanded diff lines
for the same diff file, just the _first_ expansion would be
considered, therefore, just the _first_ suggestion within
this set of comments on expanded lines would be
appliable.

All in all, we're able to simply remove this check from the
`appliable?` method given we already check if the note supports
suggestions uppon creation (`Suggestions::CreateService`).

As an extra outcome, we'll submit less Gitaly requests.
parent 0a62fd23
...@@ -75,6 +75,10 @@ class DiffNote < Note ...@@ -75,6 +75,10 @@ class DiffNote < Note
self.original_position.diff_refs == diff_refs self.original_position.diff_refs == diff_refs
end 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? def supports_suggestion?
return false unless noteable&.supports_suggestion? && on_text? return false unless noteable&.supports_suggestion? && on_text?
# We don't want to trigger side-effects of `diff_file` call. # We don't want to trigger side-effects of `diff_file` call.
......
...@@ -41,7 +41,6 @@ class Suggestion < ApplicationRecord ...@@ -41,7 +41,6 @@ class Suggestion < ApplicationRecord
!applied? && !applied? &&
noteable.opened? && noteable.opened? &&
!outdated?(cached: cached) && !outdated?(cached: cached) &&
note.supports_suggestion? &&
different_content? && different_content? &&
note.active? note.active?
end end
......
---
title: Adjust unnapliable suggestions in expanded lines
merge_request: 17286
author:
type: fixed
...@@ -118,8 +118,14 @@ module Gitlab ...@@ -118,8 +118,14 @@ module Gitlab
path: file_path path: file_path
} }
# Takes action when creating diff notes (multiple calls are
# submitted to this method).
Gitlab::SafeRequestStore.fetch(key) { find_diff_file(repository) } Gitlab::SafeRequestStore.fetch(key) { find_diff_file(repository) }
end 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 end
def diff_options def diff_options
...@@ -152,13 +158,7 @@ module Gitlab ...@@ -152,13 +158,7 @@ module Gitlab
return unless diff_refs.complete? return unless diff_refs.complete?
return unless comparison = diff_refs.compare_in(repository.project) return unless comparison = diff_refs.compare_in(repository.project)
file = comparison.diffs(diff_options).diff_files.first 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
end end
def get_formatter_class(type) def get_formatter_class(type)
......
...@@ -14,6 +14,10 @@ describe 'User comments on a diff', :js do ...@@ -14,6 +14,10 @@ describe 'User comments on a diff', :js do
expect(suggested_content).to eq(expected_suggested_content) expect(suggested_content).to eq(expected_suggested_content)
end end
def expect_appliable_suggestions(amount)
expect(all('button', text: 'Apply suggestion').size).to eq(amount)
end
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:merge_request) do let(:merge_request) do
create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test') 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 ...@@ -89,6 +93,60 @@ describe 'User comments on a diff', :js do
end end
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 context 'multiple suggestions in a single note' do
it 'suggestions are presented' do it 'suggestions are presented' do
click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
......
...@@ -130,6 +130,26 @@ describe Gitlab::Diff::Position do ...@@ -130,6 +130,26 @@ describe Gitlab::Diff::Position do
expect(diff_file.new_path).to eq(subject.new_path) expect(diff_file.new_path).to eq(subject.new_path)
expect(diff_file.diff_refs).to eq(subject.diff_refs) expect(diff_file.diff_refs).to eq(subject.diff_refs)
end 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 end
describe "#diff_line" do describe "#diff_line" do
......
...@@ -38,16 +38,6 @@ describe Suggestion do ...@@ -38,16 +38,6 @@ describe Suggestion do
end end
describe '#appliable?' do 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 context 'when patch is already applied' do
let(:suggestion) { create(:suggestion, :applied) } let(:suggestion) { create(:suggestion, :applied) }
......
...@@ -92,7 +92,7 @@ eos ...@@ -92,7 +92,7 @@ eos
) )
end end
def sample_compare def sample_compare(extra_changes = [])
changes = [ changes = [
{ {
line_code: 'a5cc2925ca8258af241be7e5b0381edf30266302_20_20', line_code: 'a5cc2925ca8258af241be7e5b0381edf30266302_20_20',
...@@ -102,7 +102,7 @@ eos ...@@ -102,7 +102,7 @@ eos
line_code: '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_4_6', line_code: '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_4_6',
file_path: '.gitmodules' file_path: '.gitmodules'
} }
] ] + extra_changes
commits = %w( commits = %w(
5937ac0a7beb003549fc5fd26fc247adbce4a52e 5937ac0a7beb003549fc5fd26fc247adbce4a52e
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment