Commit 37907aa7 authored by Robert May's avatar Robert May Committed by Robert May

Update diff discussion positions on load

Ensures we generate DiffNotePosition records on-demand for records
that need them.
parent a3516b85
...@@ -8,6 +8,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -8,6 +8,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
before_action :commit before_action :commit
before_action :define_diff_vars before_action :define_diff_vars
before_action :define_diff_comment_vars, except: [:diffs_batch, :diffs_metadata] before_action :define_diff_comment_vars, except: [:diffs_batch, :diffs_metadata]
before_action :update_diff_discussion_positions!
around_action :allow_gitaly_ref_name_caching around_action :allow_gitaly_ref_name_caching
...@@ -171,4 +172,12 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -171,4 +172,12 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
@notes.concat(draft_notes) @notes.concat(draft_notes)
end end
def update_diff_discussion_positions!
return unless Feature.enabled?(:merge_ref_head_comments, @merge_request.target_project, default_enabled: true)
return unless Feature.enabled?(:merge_red_head_comments_position_on_demand, @merge_request.target_project, default_enabled: true)
return if @merge_request.has_any_diff_note_positions?
Discussions::CaptureDiffNotePositionsService.new(@merge_request).execute
end
end end
...@@ -67,6 +67,10 @@ module Noteable ...@@ -67,6 +67,10 @@ module Noteable
false false
end end
def has_any_diff_note_positions?
notes.any? && DiffNotePosition.where(note: notes).exists?
end
def discussion_notes def discussion_notes
notes notes
end end
......
---
title: Update diff discussion positions on demand
merge_request: 34148
author:
type: added
...@@ -91,6 +91,17 @@ RSpec.describe Projects::MergeRequests::DiffsController do ...@@ -91,6 +91,17 @@ RSpec.describe Projects::MergeRequests::DiffsController do
end end
end end
shared_examples "diff note on-demand position creation" do
it "updates diff discussion positions" do
service = double("service")
expect(Discussions::CaptureDiffNotePositionsService).to receive(:new).with(merge_request).and_return(service)
expect(service).to receive(:execute)
go
end
end
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
...@@ -146,6 +157,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do ...@@ -146,6 +157,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do
it_behaves_like 'persisted preferred diff view cookie' it_behaves_like 'persisted preferred diff view cookie'
it_behaves_like 'cached diff collection' it_behaves_like 'cached diff collection'
it_behaves_like 'diff note on-demand position creation'
end end
describe 'GET diffs_metadata' do describe 'GET diffs_metadata' do
......
...@@ -262,4 +262,44 @@ describe Noteable do ...@@ -262,4 +262,44 @@ describe Noteable do
end end
end end
end end
describe "#has_any_diff_note_positions?" do
let(:source_branch) { "compare-with-merge-head-source" }
let(:target_branch) { "compare-with-merge-head-target" }
let(:merge_request) { create(:merge_request, source_branch: source_branch, target_branch: target_branch) }
let!(:note) do
path = "files/markdown/ruby-style-guide.md"
position = Gitlab::Diff::Position.new(
old_path: path,
new_path: path,
new_line: 508,
diff_refs: merge_request.diff_refs
)
create(:diff_note_on_merge_request, project: merge_request.project, position: position, noteable: merge_request)
end
before do
MergeRequests::MergeToRefService.new(merge_request.project, merge_request.author).execute(merge_request)
Discussions::CaptureDiffNotePositionsService.new(merge_request).execute
end
it "returns true when it has diff note positions" do
expect(merge_request.has_any_diff_note_positions?).to be(true)
end
it "returns false when it has notes but no diff note positions" do
DiffNotePosition.where(note: note).find_each(&:delete)
expect(merge_request.has_any_diff_note_positions?).to be(false)
end
it "returns false when it has no notes" do
merge_request.notes.find_each(&:destroy)
expect(merge_request.has_any_diff_note_positions?).to be(false)
end
end
end end
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