Commit f9b4130b authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Comment on any expanded diff line on MRs

parent 5b270431
...@@ -65,7 +65,13 @@ export default { ...@@ -65,7 +65,13 @@ export default {
const { highlightedDiffLines, parallelDiffLines } = diffFile; const { highlightedDiffLines, parallelDiffLines } = diffFile;
removeMatchLine(diffFile, lineNumbers, bottom); removeMatchLine(diffFile, lineNumbers, bottom);
const lines = addLineReferences(contextLines, lineNumbers, bottom);
const lines = addLineReferences(contextLines, lineNumbers, bottom).map(line => ({
...line,
lineCode: line.lineCode || `${fileHash}_${line.oldLine}_${line.newLine}`,
discussions: line.discussions || [],
}));
addContextLines({ addContextLines({
inlineLines: highlightedDiffLines, inlineLines: highlightedDiffLines,
parallelLines: parallelDiffLines, parallelLines: parallelDiffLines,
......
...@@ -122,7 +122,7 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -122,7 +122,7 @@ class Projects::BlobController < Projects::ApplicationController
@lines.map! do |line| @lines.map! do |line|
# These are marked as context lines but are loaded from blobs. # These are marked as context lines but are loaded from blobs.
# We also have context lines loaded from diffs in other places. # We also have context lines loaded from diffs in other places.
diff_line = Gitlab::Diff::Line.new(line, 'context', nil, nil, nil) diff_line = Gitlab::Diff::Line.new(line, expanded_diff_line_type, nil, nil, nil)
diff_line.rich_text = line diff_line.rich_text = line
diff_line diff_line
end end
...@@ -132,6 +132,11 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -132,6 +132,11 @@ class Projects::BlobController < Projects::ApplicationController
render json: DiffLineSerializer.new.represent(@lines) render json: DiffLineSerializer.new.represent(@lines)
end end
def expanded_diff_line_type
# Context lines can't receive comments.
Feature.enabled?(:comment_in_any_diff_line, @project) ? nil : 'context'
end
def add_match_line def add_match_line
return unless @form.unfold? return unless @form.unfold?
......
...@@ -22,6 +22,12 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -22,6 +22,12 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
def render_diffs def render_diffs
@environment = @merge_request.environments_for(current_user).last @environment = @merge_request.environments_for(current_user).last
notes_grouped_by_path = renderable_notes.group_by { |note| note.position.file_path }
@diffs.diff_files.each do |diff_file|
notes = notes_grouped_by_path.fetch(diff_file.file_path, [])
notes.each { |note| diff_file.unfold_diff_lines(note.position) }
end
@diffs.write_cache @diffs.write_cache
...@@ -108,4 +114,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -108,4 +114,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
@grouped_diff_discussions = @merge_request.grouped_diff_discussions(@compare.diff_refs) @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@compare.diff_refs)
@notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes), @merge_request) @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes), @merge_request)
end end
def renderable_notes
define_diff_comment_vars unless @notes
@notes
end
end end
...@@ -66,6 +66,10 @@ class DiffNote < Note ...@@ -66,6 +66,10 @@ class DiffNote < Note
self.original_position.diff_refs == diff_refs self.original_position.diff_refs == diff_refs
end end
def discussion_first_note?
self == discussion.first_note
end
private private
def enqueue_diff_file_creation_job def enqueue_diff_file_creation_job
...@@ -78,26 +82,33 @@ class DiffNote < Note ...@@ -78,26 +82,33 @@ class DiffNote < Note
end end
def should_create_diff_file? def should_create_diff_file?
on_text? && note_diff_file.nil? && self == discussion.first_note on_text? && note_diff_file.nil? && discussion_first_note?
end end
def fetch_diff_file def fetch_diff_file
if note_diff_file file =
diff = Gitlab::Git::Diff.new(note_diff_file.to_hash) if note_diff_file
Gitlab::Diff::File.new(diff, diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
repository: project.repository, Gitlab::Diff::File.new(diff,
diff_refs: original_position.diff_refs) repository: project.repository,
elsif created_at_diff?(noteable.diff_refs) diff_refs: original_position.diff_refs)
# We're able to use the already persisted diffs (Postgres) if we're elsif created_at_diff?(noteable.diff_refs)
# presenting a "current version" of the MR discussion diff. # We're able to use the already persisted diffs (Postgres) if we're
# So no need to make an extra Gitaly diff request for it. # presenting a "current version" of the MR discussion diff.
# As an extra benefit, the returned `diff_file` already # So no need to make an extra Gitaly diff request for it.
# has `highlighted_diff_lines` data set from Redis on # As an extra benefit, the returned `diff_file` already
# `Diff::FileCollection::MergeRequestDiff`. # has `highlighted_diff_lines` data set from Redis on
noteable.diffs(original_position.diff_options).diff_files.first # `Diff::FileCollection::MergeRequestDiff`.
else noteable.diffs(original_position.diff_options).diff_files.first
original_position.diff_file(self.project.repository) else
end original_position.diff_file(self.project.repository)
end
# Since persisted diff files already have its content "unfolded"
# there's no need to make it pass through the unfolding process.
file&.unfold_diff_lines(position) unless note_diff_file
file
end end
def supported? def supported?
......
...@@ -29,10 +29,6 @@ module MergeRequests ...@@ -29,10 +29,6 @@ module MergeRequests
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def clear_cache(new_diff) def clear_cache(new_diff)
# Executing the iteration we cache highlighted diffs for each diff file of
# MergeRequestDiff.
cacheable_collection(new_diff).write_cache
# Remove cache for all diffs on this MR. Do not use the association on the # Remove cache for all diffs on this MR. Do not use the association on the
# model, as that will interfere with other actions happening when # model, as that will interfere with other actions happening when
# reloading the diff. # reloading the diff.
......
# frozen_string_literal: true
module Notes
class BaseService < ::BaseService
def clear_noteable_diffs_cache(note)
if note.is_a?(DiffNote) &&
note.discussion_first_note? &&
note.position.unfolded_diff?(project.repository)
note.noteable.diffs.clear_cache
end
end
end
end
# frozen_string_literal: true # frozen_string_literal: true
module Notes module Notes
class CreateService < ::BaseService class CreateService < ::Notes::BaseService
def execute def execute
merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha) merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
...@@ -35,6 +35,7 @@ module Notes ...@@ -35,6 +35,7 @@ module Notes
if !only_commands && note.save if !only_commands && note.save
todo_service.new_note(note, current_user) todo_service.new_note(note, current_user)
clear_noteable_diffs_cache(note)
end end
if command_params.present? if command_params.present?
......
# frozen_string_literal: true # frozen_string_literal: true
module Notes module Notes
class DestroyService < BaseService class DestroyService < ::Notes::BaseService
def execute(note) def execute(note)
TodoService.new.destroy_target(note) do |note| TodoService.new.destroy_target(note) do |note|
note.destroy note.destroy
end end
clear_noteable_diffs_cache(note)
end end
end end
end end
---
title: Allow commenting on any diff line in Merge Requests
merge_request: 22914
author:
type: added
...@@ -26,6 +26,7 @@ module Gitlab ...@@ -26,6 +26,7 @@ module Gitlab
@repository = repository @repository = repository
@diff_refs = diff_refs @diff_refs = diff_refs
@fallback_diff_refs = fallback_diff_refs @fallback_diff_refs = fallback_diff_refs
@unfolded = false
# Ensure items are collected in the the batch # Ensure items are collected in the the batch
new_blob_lazy new_blob_lazy
...@@ -135,6 +136,24 @@ module Gitlab ...@@ -135,6 +136,24 @@ module Gitlab
Gitlab::Diff::Parser.new.parse(raw_diff.each_line, diff_file: self).to_a Gitlab::Diff::Parser.new.parse(raw_diff.each_line, diff_file: self).to_a
end end
# Changes diff_lines according to the given position. That is,
# it checks whether the position requires blob lines into the diff
# in order to be presented.
def unfold_diff_lines(position)
return unless position
unfolder = Gitlab::Diff::LinesUnfolder.new(self, position)
if unfolder.unfold_required?
@diff_lines = unfolder.unfolded_diff_lines
@unfolded = true
end
end
def unfolded?
@unfolded
end
def highlighted_diff_lines def highlighted_diff_lines
@highlighted_diff_lines ||= @highlighted_diff_lines ||=
Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
......
...@@ -3,9 +3,9 @@ module Gitlab ...@@ -3,9 +3,9 @@ module Gitlab
class Line class Line
SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze
attr_reader :line_code, :type, :index, :old_pos, :new_pos attr_reader :line_code, :type, :old_pos, :new_pos
attr_writer :rich_text attr_writer :rich_text
attr_accessor :text attr_accessor :text, :index
def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil) def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil)
@text, @type, @index = text, type, index @text, @type, @index = text, type, index
...@@ -19,7 +19,14 @@ module Gitlab ...@@ -19,7 +19,14 @@ module Gitlab
end end
def self.init_from_hash(hash) def self.init_from_hash(hash)
new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos], line_code: hash[:line_code], rich_text: hash[:rich_text]) new(hash[:text],
hash[:type],
hash[:index],
hash[:old_pos],
hash[:new_pos],
parent_file: hash[:parent_file],
line_code: hash[:line_code],
rich_text: hash[:rich_text])
end end
def to_hash def to_hash
......
# frozen_string_literal: true
# Given a position, calculates which Blob lines should be extracted, treated and
# injected in the current diff file lines in order to present a "unfolded" diff.
module Gitlab
module Diff
class LinesUnfolder
include Gitlab::Utils::StrongMemoize
UNFOLD_CONTEXT_SIZE = 3
def initialize(diff_file, position)
@diff_file = diff_file
@blob = diff_file.old_blob
@position = position
@generate_top_match_line = true
@generate_bottom_match_line = true
# These methods update `@generate_top_match_line` and
# `@generate_bottom_match_line`.
@from_blob_line = calculate_from_blob_line!
@to_blob_line = calculate_to_blob_line!
end
# Returns merged diff lines with required blob lines with correct
# positions.
def unfolded_diff_lines
strong_memoize(:unfolded_diff_lines) do
next unless unfold_required?
merged_diff_with_blob_lines
end
end
# Returns the extracted lines from the old blob which should be merged
# with the current diff lines.
def blob_lines
strong_memoize(:blob_lines) do
# Blob lines, unlike diffs, doesn't start with an empty space for
# unchanged line, so the parsing and highlighting step can get fuzzy
# without the following change.
line_prefix = ' '
blob_as_diff_lines = @blob.data.each_line.map { |line| "#{line_prefix}#{line}" }
lines = Gitlab::Diff::Parser.new.parse(blob_as_diff_lines, diff_file: @diff_file).to_a
from = from_blob_line - 1
to = to_blob_line - 1
lines[from..to]
end
end
def unfold_required?
strong_memoize(:unfold_required) do
next false unless @diff_file.text?
next false unless @position.unchanged?
next false if @diff_file.new_file? || @diff_file.deleted_file?
next false unless @position.old_line
# Invalid position (MR import scenario)
next false if @position.old_line > @blob.lines.size
next false if @diff_file.diff_lines.empty?
next false if @diff_file.line_for_position(@position)
next false unless unfold_line
true
end
end
private
attr_reader :from_blob_line, :to_blob_line
def merged_diff_with_blob_lines
lines = @diff_file.diff_lines
match_line = unfold_line
insert_index = bottom? ? -1 : match_line.index
lines -= [match_line] unless bottom?
lines.insert(insert_index, *blob_lines_with_matches)
# The inserted blob lines have invalid indexes, so we need
# to reindex them.
reindex(lines)
lines
end
# Returns 'unchanged' blob lines with recalculated `old_pos` and
# `new_pos` and the recalculated new match line (needed if we for instance
# we unfolded once, but there are still folded lines).
def blob_lines_with_matches
old_pos = from_blob_line
new_pos = from_blob_line + offset
new_blob_lines = []
new_blob_lines.push(top_blob_match_line) if top_blob_match_line
blob_lines.each do |line|
new_blob_lines << Gitlab::Diff::Line.new(line.text, line.type, nil, old_pos, new_pos,
parent_file: @diff_file)
old_pos += 1
new_pos += 1
end
new_blob_lines.push(bottom_blob_match_line) if bottom_blob_match_line
new_blob_lines
end
def reindex(lines)
lines.each_with_index { |line, i| line.index = i }
end
def top_blob_match_line
strong_memoize(:top_blob_match_line) do
next unless @generate_top_match_line
old_pos = from_blob_line
new_pos = from_blob_line + offset
build_match_line(old_pos, new_pos)
end
end
def bottom_blob_match_line
strong_memoize(:bottom_blob_match_line) do
# The bottom line match addition is already handled on
# Diff::File#diff_lines_for_serializer
next if bottom?
next unless @generate_bottom_match_line
position = line_after_unfold_position.old_pos
old_pos = position
new_pos = position + offset
build_match_line(old_pos, new_pos)
end
end
def build_match_line(old_pos, new_pos)
blob_lines_length = blob_lines.length
old_line_ref = [old_pos, blob_lines_length].join(',')
new_line_ref = [new_pos, blob_lines_length].join(',')
new_match_line_str = "@@ -#{old_line_ref}+#{new_line_ref} @@"
Gitlab::Diff::Line.new(new_match_line_str, 'match', nil, old_pos, new_pos)
end
# Returns the first line position that should be extracted
# from `blob_lines`.
def calculate_from_blob_line!
return unless unfold_required?
from = comment_position - UNFOLD_CONTEXT_SIZE
# There's no line before the match if it's in the top-most
# position.
prev_line_number = line_before_unfold_position&.old_pos || 0
if from <= prev_line_number + 1
@generate_top_match_line = false
from = prev_line_number + 1
end
from
end
# Returns the last line position that should be extracted
# from `blob_lines`.
def calculate_to_blob_line!
return unless unfold_required?
to = comment_position + UNFOLD_CONTEXT_SIZE
return to if bottom?
next_line_number = line_after_unfold_position.old_pos
if to >= next_line_number - 1
@generate_bottom_match_line = false
to = next_line_number - 1
end
to
end
def offset
unfold_line.new_pos - unfold_line.old_pos
end
def line_before_unfold_position
return unless index = unfold_line&.index
@diff_file.diff_lines[index - 1] if index > 0
end
def line_after_unfold_position
return unless index = unfold_line&.index
@diff_file.diff_lines[index + 1] if index >= 0
end
def bottom?
strong_memoize(:bottom) do
@position.old_line > last_line.old_pos
end
end
# Returns the line which needed to be expanded in order to send a comment
# in `@position`.
def unfold_line
strong_memoize(:unfold_line) do
next last_line if bottom?
@diff_file.diff_lines.find do |line|
line.old_pos > comment_position && line.type == 'match'
end
end
end
def comment_position
@position.old_line
end
def last_line
@diff_file.diff_lines.last
end
end
end
end
...@@ -101,6 +101,10 @@ module Gitlab ...@@ -101,6 +101,10 @@ module Gitlab
@diff_refs ||= DiffRefs.new(base_sha: base_sha, start_sha: start_sha, head_sha: head_sha) @diff_refs ||= DiffRefs.new(base_sha: base_sha, start_sha: start_sha, head_sha: head_sha)
end end
def unfolded_diff?(repository)
diff_file(repository)&.unfolded?
end
def diff_file(repository) def diff_file(repository)
return @diff_file if defined?(@diff_file) return @diff_file if defined?(@diff_file)
...@@ -134,7 +138,13 @@ module Gitlab ...@@ -134,7 +138,13 @@ 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)
comparison.diffs(diff_options).diff_files.first 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
end end
def get_formatter_class(type) def get_formatter_class(type)
......
...@@ -141,6 +141,28 @@ describe Projects::BlobController do ...@@ -141,6 +141,28 @@ describe Projects::BlobController do
expect(lines.first).to have_key('rich_text') expect(lines.first).to have_key('rich_text')
end end
context 'comment in any diff line feature flag' do
it 'renders context lines when feature disabled' do
stub_feature_flags(comment_in_any_diff_line: false)
do_get(since: 1, to: 5, offset: 10, from_merge_request: true)
lines = JSON.parse(response.body)
all_context = lines.all? { |line| line['type'] == 'context' }
expect(all_context).to be(true)
end
it 'renders unchanged lines when feature enabled' do
stub_feature_flags(comment_in_any_diff_line: true)
do_get(since: 1, to: 5, offset: 10, from_merge_request: true)
lines = JSON.parse(response.body)
all_unchanged = lines.all? { |line| line['type'].nil? }
expect(all_unchanged).to be(true)
end
end
context 'when rendering match lines' do context 'when rendering match lines' do
it 'adds top match line when "since" is less than 1' do it 'adds top match line when "since" is less than 1' do
do_get(since: 5, to: 10, offset: 10, from_merge_request: true) do_get(since: 5, to: 10, offset: 10, from_merge_request: true)
...@@ -157,7 +179,7 @@ describe Projects::BlobController do ...@@ -157,7 +179,7 @@ describe Projects::BlobController do
match_line = JSON.parse(response.body).first match_line = JSON.parse(response.body).first
expect(match_line['type']).to eq('context') expect(match_line['type']).to be_nil
end end
it 'adds bottom match line when "t"o is less than blob size' do it 'adds bottom match line when "t"o is less than blob size' do
...@@ -177,7 +199,7 @@ describe Projects::BlobController do ...@@ -177,7 +199,7 @@ describe Projects::BlobController do
match_line = JSON.parse(response.body).last match_line = JSON.parse(response.body).last
expect(match_line['type']).to eq('context') expect(match_line['type']).to be_nil
end end
end end
end end
......
...@@ -85,12 +85,13 @@ describe 'Merge request > User posts diff notes', :js do ...@@ -85,12 +85,13 @@ describe 'Merge request > User posts diff notes', :js do
# `.line_holder` will be an unfolded line. # `.line_holder` will be an unfolded line.
let(:line_holder) { first('#a5cc2925ca8258af241be7e5b0381edf30266302 .line_holder') } let(:line_holder) { first('#a5cc2925ca8258af241be7e5b0381edf30266302 .line_holder') }
it 'does not allow commenting on the left side' do it 'allows commenting on the left side' do
should_not_allow_commenting(line_holder, 'left') should_allow_commenting(line_holder, 'left')
end end
it 'does not allow commenting on the right side' do it 'allows commenting on the right side' do
should_not_allow_commenting(line_holder, 'right') # Automatically shifts comment box to left side.
should_allow_commenting(line_holder, 'right')
end end
end end
end end
...@@ -147,8 +148,8 @@ describe 'Merge request > User posts diff notes', :js do ...@@ -147,8 +148,8 @@ describe 'Merge request > User posts diff notes', :js do
# `.line_holder` will be an unfolded line. # `.line_holder` will be an unfolded line.
let(:line_holder) { first('.line_holder[id="a5cc2925ca8258af241be7e5b0381edf30266302_1_1"]') } let(:line_holder) { first('.line_holder[id="a5cc2925ca8258af241be7e5b0381edf30266302_1_1"]') }
it 'does not allow commenting' do it 'allows commenting' do
should_not_allow_commenting line_holder should_allow_commenting line_holder
end end
end end
......
...@@ -98,7 +98,7 @@ describe('DiffsStoreMutations', () => { ...@@ -98,7 +98,7 @@ describe('DiffsStoreMutations', () => {
it('should call utils.addContextLines with proper params', () => { it('should call utils.addContextLines with proper params', () => {
const options = { const options = {
lineNumbers: { oldLineNumber: 1, newLineNumber: 2 }, lineNumbers: { oldLineNumber: 1, newLineNumber: 2 },
contextLines: [{ oldLine: 1 }], contextLines: [{ oldLine: 1, newLine: 1, lineCode: 'ff9200_1_1', discussions: [] }],
fileHash: 'ff9200', fileHash: 'ff9200',
params: { params: {
bottom: true, bottom: true,
...@@ -110,7 +110,7 @@ describe('DiffsStoreMutations', () => { ...@@ -110,7 +110,7 @@ describe('DiffsStoreMutations', () => {
parallelDiffLines: [], parallelDiffLines: [],
}; };
const state = { diffFiles: [diffFile] }; const state = { diffFiles: [diffFile] };
const lines = [{ oldLine: 1 }]; const lines = [{ oldLine: 1, newLine: 1 }];
const findDiffFileSpy = spyOnDependency(mutations, 'findDiffFile').and.returnValue(diffFile); const findDiffFileSpy = spyOnDependency(mutations, 'findDiffFile').and.returnValue(diffFile);
const removeMatchLineSpy = spyOnDependency(mutations, 'removeMatchLine'); const removeMatchLineSpy = spyOnDependency(mutations, 'removeMatchLine');
......
...@@ -41,6 +41,52 @@ describe Gitlab::Diff::File do ...@@ -41,6 +41,52 @@ describe Gitlab::Diff::File do
end end
end end
describe '#unfold_diff_lines' do
let(:unfolded_lines) { double('expanded-lines') }
let(:unfolder) { instance_double(Gitlab::Diff::LinesUnfolder) }
let(:position) { instance_double(Gitlab::Diff::Position, old_line: 10) }
before do
allow(Gitlab::Diff::LinesUnfolder).to receive(:new) { unfolder }
end
context 'when unfold required' do
before do
allow(unfolder).to receive(:unfold_required?) { true }
allow(unfolder).to receive(:unfolded_diff_lines) { unfolded_lines }
end
it 'changes @unfolded to true' do
diff_file.unfold_diff_lines(position)
expect(diff_file).to be_unfolded
end
it 'updates @diff_lines' do
diff_file.unfold_diff_lines(position)
expect(diff_file.diff_lines).to eq(unfolded_lines)
end
end
context 'when unfold not required' do
before do
allow(unfolder).to receive(:unfold_required?) { false }
end
it 'keeps @unfolded false' do
diff_file.unfold_diff_lines(position)
expect(diff_file).not_to be_unfolded
end
it 'does not update @diff_lines' do
expect { diff_file.unfold_diff_lines(position) }
.not_to change(diff_file, :diff_lines)
end
end
end
describe '#mode_changed?' do describe '#mode_changed?' do
it { expect(diff_file.mode_changed?).to be_falsey } it { expect(diff_file.mode_changed?).to be_falsey }
end end
......
This diff is collapsed.
...@@ -31,32 +31,11 @@ describe MergeRequests::ReloadDiffsService, :use_clean_rails_memory_store_cachin ...@@ -31,32 +31,11 @@ describe MergeRequests::ReloadDiffsService, :use_clean_rails_memory_store_cachin
end end
context 'cache clearing' do context 'cache clearing' do
before do
allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(true)
allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(true)
end
it 'retrieves the diff files to cache the highlighted result' do
new_diff = merge_request.create_merge_request_diff
cache_key = new_diff.diffs_collection.cache_key
expect(merge_request).to receive(:create_merge_request_diff).and_return(new_diff)
expect(Rails.cache).to receive(:read).with(cache_key).and_call_original
expect(Rails.cache).to receive(:write).with(cache_key, anything, anything).and_call_original
subject.execute
end
it 'clears the cache for older diffs on the merge request' do it 'clears the cache for older diffs on the merge request' do
old_diff = merge_request.merge_request_diff old_diff = merge_request.merge_request_diff
old_cache_key = old_diff.diffs_collection.cache_key old_cache_key = old_diff.diffs_collection.cache_key
new_diff = merge_request.create_merge_request_diff
new_cache_key = new_diff.diffs_collection.cache_key
expect(merge_request).to receive(:create_merge_request_diff).and_return(new_diff)
expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original
expect(Rails.cache).to receive(:read).with(new_cache_key).and_call_original
expect(Rails.cache).to receive(:write).with(new_cache_key, anything, anything).and_call_original
subject.execute subject.execute
end end
......
...@@ -57,6 +57,57 @@ describe Notes::CreateService do ...@@ -57,6 +57,57 @@ describe Notes::CreateService do
end end
end end
context 'noteable highlight cache clearing' do
let(:project_with_repo) { create(:project, :repository) }
let(:merge_request) do
create(:merge_request, source_project: project_with_repo,
target_project: project_with_repo)
end
let(:position) do
Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 14,
diff_refs: merge_request.diff_refs)
end
let(:new_opts) do
opts.merge(in_reply_to_discussion_id: nil,
type: 'DiffNote',
noteable_type: 'MergeRequest',
noteable_id: merge_request.id,
position: position.to_h)
end
before do
allow_any_instance_of(Gitlab::Diff::Position)
.to receive(:unfolded_diff?) { true }
end
it 'clears noteable diff cache when it was unfolded for the note position' do
expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear)
described_class.new(project_with_repo, user, new_opts).execute
end
it 'does not clear cache when note is not the first of the discussion' do
prev_note =
create(:diff_note_on_merge_request, noteable: merge_request,
project: project_with_repo)
reply_opts =
opts.merge(in_reply_to_discussion_id: prev_note.discussion_id,
type: 'DiffNote',
noteable_type: 'MergeRequest',
noteable_id: merge_request.id,
position: position.to_h)
expect(merge_request).not_to receive(:diffs)
described_class.new(project_with_repo, user, reply_opts).execute
end
end
context 'note diff file' do context 'note diff file' do
let(:project_with_repo) { create(:project, :repository) } let(:project_with_repo) { create(:project, :repository) }
let(:merge_request) do let(:merge_request) do
......
...@@ -21,5 +21,38 @@ describe Notes::DestroyService do ...@@ -21,5 +21,38 @@ describe Notes::DestroyService do
expect { described_class.new(project, user).execute(note) } expect { described_class.new(project, user).execute(note) }
.to change { user.todos_pending_count }.from(1).to(0) .to change { user.todos_pending_count }.from(1).to(0)
end end
context 'noteable highlight cache clearing' do
let(:repo_project) { create(:project, :repository) }
let(:merge_request) do
create(:merge_request, source_project: repo_project,
target_project: repo_project)
end
let(:note) do
create(:diff_note_on_merge_request, project: repo_project,
noteable: merge_request)
end
before do
allow(note.position).to receive(:unfolded_diff?) { true }
end
it 'clears noteable diff cache when it was unfolded for the note position' do
expect(merge_request).to receive_message_chain(:diffs, :clear_cache)
described_class.new(repo_project, user).execute(note)
end
it 'does not clear cache when note is not the first of the discussion' do
reply_note = create(:diff_note_on_merge_request, in_reply_to: note,
project: repo_project,
noteable: merge_request)
expect(merge_request).not_to receive(:diffs)
described_class.new(repo_project, user).execute(reply_note)
end
end
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