Commit 198402b3 authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Revert "Merge branch 'osw-comment-on-any-line-on-diffs' into 'master'"

This reverts commit a82a5957, reversing
changes made to e7df959b.
parent 8467167b
...@@ -55,6 +55,11 @@ export default { ...@@ -55,6 +55,11 @@ export default {
required: false, required: false,
default: false, default: false,
}, },
isContextLine: {
type: Boolean,
required: false,
default: false,
},
isHover: { isHover: {
type: Boolean, type: Boolean,
required: false, required: false,
...@@ -76,6 +81,7 @@ export default { ...@@ -76,6 +81,7 @@ export default {
this.showCommentButton && this.showCommentButton &&
this.isHover && this.isHover &&
!this.isMatchLine && !this.isMatchLine &&
!this.isContextLine &&
!this.isMetaLine && !this.isMetaLine &&
!this.hasDiscussions !this.hasDiscussions
); );
......
...@@ -3,6 +3,7 @@ import { mapGetters } from 'vuex'; ...@@ -3,6 +3,7 @@ import { mapGetters } from 'vuex';
import DiffLineGutterContent from './diff_line_gutter_content.vue'; import DiffLineGutterContent from './diff_line_gutter_content.vue';
import { import {
MATCH_LINE_TYPE, MATCH_LINE_TYPE,
CONTEXT_LINE_TYPE,
EMPTY_CELL_TYPE, EMPTY_CELL_TYPE,
OLD_LINE_TYPE, OLD_LINE_TYPE,
OLD_NO_NEW_LINE_TYPE, OLD_NO_NEW_LINE_TYPE,
...@@ -70,6 +71,9 @@ export default { ...@@ -70,6 +71,9 @@ export default {
isMatchLine() { isMatchLine() {
return this.line.type === MATCH_LINE_TYPE; return this.line.type === MATCH_LINE_TYPE;
}, },
isContextLine() {
return this.line.type === CONTEXT_LINE_TYPE;
},
isMetaLine() { isMetaLine() {
const { type } = this.line; const { type } = this.line;
...@@ -84,7 +88,11 @@ export default { ...@@ -84,7 +88,11 @@ export default {
[type]: type, [type]: type,
[LINE_UNFOLD_CLASS_NAME]: this.isMatchLine, [LINE_UNFOLD_CLASS_NAME]: this.isMatchLine,
[LINE_HOVER_CLASS_NAME]: [LINE_HOVER_CLASS_NAME]:
this.isLoggedIn && this.isHover && !this.isMatchLine && !this.isMetaLine, this.isLoggedIn &&
this.isHover &&
!this.isMatchLine &&
!this.isContextLine &&
!this.isMetaLine,
}; };
}, },
lineNumber() { lineNumber() {
......
...@@ -4,6 +4,8 @@ import DiffTableCell from './diff_table_cell.vue'; ...@@ -4,6 +4,8 @@ import DiffTableCell from './diff_table_cell.vue';
import { import {
NEW_LINE_TYPE, NEW_LINE_TYPE,
OLD_LINE_TYPE, OLD_LINE_TYPE,
CONTEXT_LINE_TYPE,
CONTEXT_LINE_CLASS_NAME,
PARALLEL_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE,
LINE_POSITION_LEFT, LINE_POSITION_LEFT,
LINE_POSITION_RIGHT, LINE_POSITION_RIGHT,
...@@ -39,9 +41,13 @@ export default { ...@@ -39,9 +41,13 @@ export default {
}, },
computed: { computed: {
...mapGetters('diffs', ['isInlineView']), ...mapGetters('diffs', ['isInlineView']),
isContextLine() {
return this.line.type === CONTEXT_LINE_TYPE;
},
classNameMap() { classNameMap() {
return { return {
[this.line.type]: this.line.type, [this.line.type]: this.line.type,
[CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
[PARALLEL_DIFF_VIEW_TYPE]: this.isParallelView, [PARALLEL_DIFF_VIEW_TYPE]: this.isParallelView,
}; };
}, },
......
...@@ -5,6 +5,8 @@ import DiffTableCell from './diff_table_cell.vue'; ...@@ -5,6 +5,8 @@ import DiffTableCell from './diff_table_cell.vue';
import { import {
NEW_LINE_TYPE, NEW_LINE_TYPE,
OLD_LINE_TYPE, OLD_LINE_TYPE,
CONTEXT_LINE_TYPE,
CONTEXT_LINE_CLASS_NAME,
OLD_NO_NEW_LINE_TYPE, OLD_NO_NEW_LINE_TYPE,
PARALLEL_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE,
NEW_NO_NEW_LINE_TYPE, NEW_NO_NEW_LINE_TYPE,
...@@ -41,8 +43,12 @@ export default { ...@@ -41,8 +43,12 @@ export default {
}; };
}, },
computed: { computed: {
isContextLine() {
return this.line.left && this.line.left.type === CONTEXT_LINE_TYPE;
},
classNameMap() { classNameMap() {
return { return {
[CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
[PARALLEL_DIFF_VIEW_TYPE]: true, [PARALLEL_DIFF_VIEW_TYPE]: true,
}; };
}, },
......
...@@ -3,6 +3,7 @@ export const PARALLEL_DIFF_VIEW_TYPE = 'parallel'; ...@@ -3,6 +3,7 @@ export const PARALLEL_DIFF_VIEW_TYPE = 'parallel';
export const MATCH_LINE_TYPE = 'match'; export const MATCH_LINE_TYPE = 'match';
export const OLD_NO_NEW_LINE_TYPE = 'old-nonewline'; export const OLD_NO_NEW_LINE_TYPE = 'old-nonewline';
export const NEW_NO_NEW_LINE_TYPE = 'new-nonewline'; export const NEW_NO_NEW_LINE_TYPE = 'new-nonewline';
export const CONTEXT_LINE_TYPE = 'context';
export const EMPTY_CELL_TYPE = 'empty-cell'; export const EMPTY_CELL_TYPE = 'empty-cell';
export const COMMENT_FORM_TYPE = 'commentForm'; export const COMMENT_FORM_TYPE = 'commentForm';
export const DIFF_NOTE_TYPE = 'DiffNote'; export const DIFF_NOTE_TYPE = 'DiffNote';
...@@ -21,6 +22,7 @@ export const LINE_SIDE_RIGHT = 'right-side'; ...@@ -21,6 +22,7 @@ export const LINE_SIDE_RIGHT = 'right-side';
export const DIFF_VIEW_COOKIE_NAME = 'diff_view'; export const DIFF_VIEW_COOKIE_NAME = 'diff_view';
export const LINE_HOVER_CLASS_NAME = 'is-over'; export const LINE_HOVER_CLASS_NAME = 'is-over';
export const LINE_UNFOLD_CLASS_NAME = 'unfold js-unfold'; export const LINE_UNFOLD_CLASS_NAME = 'unfold js-unfold';
export const CONTEXT_LINE_CLASS_NAME = 'diff-expanded';
export const UNFOLD_COUNT = 20; export const UNFOLD_COUNT = 20;
export const COUNT_OF_AVATARS_IN_GUTTER = 3; export const COUNT_OF_AVATARS_IN_GUTTER = 3;
......
...@@ -65,13 +65,7 @@ export default { ...@@ -65,13 +65,7 @@ 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, nil, nil, nil, nil) diff_line = Gitlab::Diff::Line.new(line, 'context', nil, nil, nil)
diff_line.rich_text = line diff_line.rich_text = line
diff_line diff_line
end end
......
...@@ -22,12 +22,6 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -22,12 +22,6 @@ 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 = @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
......
...@@ -66,10 +66,6 @@ class DiffNote < Note ...@@ -66,10 +66,6 @@ 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
...@@ -82,33 +78,26 @@ class DiffNote < Note ...@@ -82,33 +78,26 @@ class DiffNote < Note
end end
def should_create_diff_file? def should_create_diff_file?
on_text? && note_diff_file.nil? && discussion_first_note? on_text? && note_diff_file.nil? && self == discussion.first_note
end end
def fetch_diff_file def fetch_diff_file
file = if note_diff_file
if note_diff_file diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
diff = Gitlab::Git::Diff.new(note_diff_file.to_hash) Gitlab::Diff::File.new(diff,
Gitlab::Diff::File.new(diff, repository: project.repository,
repository: project.repository, diff_refs: original_position.diff_refs)
diff_refs: original_position.diff_refs) elsif created_at_diff?(noteable.diff_refs)
elsif created_at_diff?(noteable.diff_refs) # We're able to use the already persisted diffs (Postgres) if we're
# We're able to use the already persisted diffs (Postgres) if we're # presenting a "current version" of the MR discussion diff.
# presenting a "current version" of the MR discussion diff. # So no need to make an extra Gitaly diff request for it.
# So no need to make an extra Gitaly diff request for it. # As an extra benefit, the returned `diff_file` already
# As an extra benefit, the returned `diff_file` already # has `highlighted_diff_lines` data set from Redis on
# has `highlighted_diff_lines` data set from Redis on # `Diff::FileCollection::MergeRequestDiff`.
# `Diff::FileCollection::MergeRequestDiff`. noteable.diffs(original_position.diff_options).diff_files.first
noteable.diffs(original_position.diff_options).diff_files.first else
else original_position.diff_file(self.project.repository)
original_position.diff_file(self.project.repository) end
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,6 +29,10 @@ module MergeRequests ...@@ -29,6 +29,10 @@ 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)
noteable = note.noteable
if note.is_a?(DiffNote) &&
note.discussion_first_note? &&
note.position.unfolded_diff?(project.repository)
noteable.diffs.clear_cache
end
end
end
end
# frozen_string_literal: true # frozen_string_literal: true
module Notes module Notes
class CreateService < ::Notes::BaseService class CreateService < ::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,7 +35,6 @@ module Notes ...@@ -35,7 +35,6 @@ 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 < ::Notes::BaseService class DestroyService < 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: 22398
author:
type: added
...@@ -26,7 +26,6 @@ module Gitlab ...@@ -26,7 +26,6 @@ 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
...@@ -136,24 +135,6 @@ module Gitlab ...@@ -136,24 +135,6 @@ 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, :old_pos, :new_pos attr_reader :line_code, :type, :index, :old_pos, :new_pos
attr_writer :rich_text attr_writer :rich_text
attr_accessor :text, :index attr_accessor :text
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,14 +19,7 @@ module Gitlab ...@@ -19,14 +19,7 @@ module Gitlab
end end
def self.init_from_hash(hash) def self.init_from_hash(hash)
new(hash[:text], new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos], line_code: hash[:line_code], rich_text: hash[:rich_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,10 +101,6 @@ module Gitlab ...@@ -101,10 +101,6 @@ 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)
...@@ -138,13 +134,7 @@ module Gitlab ...@@ -138,13 +134,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)
......
...@@ -157,7 +157,7 @@ describe Projects::BlobController do ...@@ -157,7 +157,7 @@ describe Projects::BlobController do
match_line = JSON.parse(response.body).first match_line = JSON.parse(response.body).first
expect(match_line['type']).to be_nil expect(match_line['type']).to eq('context')
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 +177,7 @@ describe Projects::BlobController do ...@@ -177,7 +177,7 @@ describe Projects::BlobController do
match_line = JSON.parse(response.body).last match_line = JSON.parse(response.body).last
expect(match_line['type']).to be_nil expect(match_line['type']).to eq('context')
end end
end end
end end
......
...@@ -85,13 +85,12 @@ describe 'Merge request > User posts diff notes', :js do ...@@ -85,13 +85,12 @@ 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 'allows commenting on the left side' do it 'does not allow commenting on the left side' do
should_allow_commenting(line_holder, 'left') should_not_allow_commenting(line_holder, 'left')
end end
it 'allows commenting on the right side' do it 'does not allow commenting on the right side' do
# Automatically shifts comment box to left side. should_not_allow_commenting(line_holder, 'right')
should_allow_commenting(line_holder, 'right')
end end
end end
end end
...@@ -148,8 +147,8 @@ describe 'Merge request > User posts diff notes', :js do ...@@ -148,8 +147,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 'allows commenting' do it 'does not allow commenting' do
should_allow_commenting line_holder should_not_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, newLine: 1, lineCode: 'ff9200_1_1', discussions: [] }], contextLines: [{ oldLine: 1 }],
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, newLine: 1 }]; const lines = [{ oldLine: 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,52 +41,6 @@ describe Gitlab::Diff::File do ...@@ -41,52 +41,6 @@ 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,11 +31,32 @@ describe MergeRequests::ReloadDiffsService, :use_clean_rails_memory_store_cachin ...@@ -31,11 +31,32 @@ 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,57 +57,6 @@ describe Notes::CreateService do ...@@ -57,57 +57,6 @@ 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,38 +21,5 @@ describe Notes::DestroyService do ...@@ -21,38 +21,5 @@ 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