Commit 6cfa5ee5 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'expand-diff-to-full-file' into 'master'

Expand diff to entire file

Closes #19054

See merge request gitlab-org/gitlab-ce!24406
parents c45bb62c cea59dbe
......@@ -5,7 +5,7 @@ import { polyfillSticky } from '~/lib/utils/sticky';
import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
import Icon from '~/vue_shared/components/icon.vue';
import FileIcon from '~/vue_shared/components/file_icon.vue';
import { GlTooltipDirective } from '@gitlab/ui';
import { GlButton, GlTooltipDirective, GlTooltip, GlLoadingIcon } from '@gitlab/ui';
import { truncateSha } from '~/lib/utils/text_utility';
import { __, s__, sprintf } from '~/locale';
import { diffViewerModes } from '~/ide/constants';
......@@ -14,6 +14,9 @@ import DiffStats from './diff_stats.vue';
export default {
components: {
GlTooltip,
GlLoadingIcon,
GlButton,
ClipboardButton,
EditButton,
Icon,
......@@ -125,12 +128,15 @@ export default {
isModeChanged() {
return this.diffFile.viewer.name === diffViewerModes.mode_changed;
},
showExpandDiffToFullFileEnabled() {
return gon.features.expandDiffFullFile && !this.diffFile.is_fully_expanded;
},
},
mounted() {
polyfillSticky(this.$refs.header);
},
methods: {
...mapActions('diffs', ['toggleFileDiscussions']),
...mapActions('diffs', ['toggleFileDiscussions', 'toggleFullDiff']),
handleToggleFile(e, checkTarget) {
if (
!checkTarget ||
......@@ -240,12 +246,30 @@ export default {
v-html="viewReplacedFileButtonText"
>
</a>
<a
<gl-tooltip :target="() => $refs.viewButton" placement="bottom">
<span v-html="viewFileButtonText"></span>
</gl-tooltip>
<gl-button
ref="viewButton"
:href="diffFile.view_path"
class="btn view-file js-view-file-button"
v-html="viewFileButtonText"
target="blank"
class="view-file js-view-file-button"
>
</a>
<icon name="external-link" />
</gl-button>
<gl-button
v-if="showExpandDiffToFullFileEnabled"
class="expand-file js-expand-file"
@click="toggleFullDiff(diffFile.file_path)"
>
<template v-if="diffFile.isShowingFullFile">
{{ s__('MRDiff|Show changes only') }}
</template>
<template v-else>
{{ s__('MRDiff|Show full file') }}
</template>
<gl-loading-icon v-if="diffFile.isLoadingFullFile" inline />
</gl-button>
<a
v-if="diffFile.external_url"
......
<script>
import { GlTooltipDirective, GlButton } from '@gitlab/ui';
import Icon from '~/vue_shared/components/icon.vue';
export default {
components: {
GlButton,
Icon,
},
directives: {
GlTooltip: GlTooltipDirective,
},
props: {
editPath: {
type: String,
......@@ -27,5 +37,13 @@ export default {
</script>
<template>
<a :href="editPath" class="btn btn-default js-edit-blob" @click="handleEditClick"> Edit </a>
<gl-button
v-gl-tooltip.bottom
:href="editPath"
:title="__('Edit file')"
class="js-edit-blob"
@click.native="handleEditClick"
>
<icon name="pencil" />
</gl-button>
</template>
......@@ -42,3 +42,8 @@ export const INITIAL_TREE_WIDTH = 320;
export const MIN_TREE_WIDTH = 240;
export const MAX_TREE_WIDTH = 400;
export const TREE_HIDE_STATS_WIDTH = 260;
export const OLD_LINE_KEY = 'old_line';
export const NEW_LINE_KEY = 'new_line';
export const TYPE_KEY = 'type';
export const LEFT_LINE_KEY = 'left';
......@@ -309,5 +309,40 @@ export const cacheTreeListWidth = (_, size) => {
localStorage.setItem(TREE_LIST_WIDTH_STORAGE_KEY, size);
};
export const requestFullDiff = ({ commit }, filePath) => commit(types.REQUEST_FULL_DIFF, filePath);
export const receiveFullDiffSucess = ({ commit }, { filePath, data }) =>
commit(types.RECEIVE_FULL_DIFF_SUCCESS, { filePath, data });
export const receiveFullDiffError = ({ commit }, filePath) => {
commit(types.RECEIVE_FULL_DIFF_ERROR, filePath);
createFlash(s__('MergeRequest|Error loading full diff. Please try again.'));
};
export const fetchFullDiff = ({ dispatch }, file) =>
axios
.get(file.context_lines_path, {
params: {
full: true,
from_merge_request: true,
},
})
.then(({ data }) => dispatch('receiveFullDiffSucess', { filePath: file.file_path, data }))
.then(() => scrollToElement(`#${file.file_hash}`))
.catch(() => dispatch('receiveFullDiffError', file.file_path));
export const toggleFullDiff = ({ dispatch, getters, state }, filePath) => {
const file = state.diffFiles.find(f => f.file_path === filePath);
dispatch('requestFullDiff', filePath);
if (file.isShowingFullFile) {
dispatch('loadCollapsedDiff', file)
.then(() => dispatch('assignDiscussionsToDiff', getters.getDiffFileDiscussions(file)))
.then(() => scrollToElement(`#${file.file_hash}`))
.catch(() => dispatch('receiveFullDiffError', filePath));
} else {
dispatch('fetchFullDiff', file);
}
};
// prevent babel-plugin-rewire from generating an invalid default during karma tests
export default () => {};
......@@ -23,3 +23,7 @@ export const SET_TREE_DATA = 'SET_TREE_DATA';
export const SET_RENDER_TREE_LIST = 'SET_RENDER_TREE_LIST';
export const SET_SHOW_WHITESPACE = 'SET_SHOW_WHITESPACE';
export const TOGGLE_FILE_FINDER_VISIBLE = 'TOGGLE_FILE_FINDER_VISIBLE';
export const REQUEST_FULL_DIFF = 'REQUEST_FULL_DIFF';
export const RECEIVE_FULL_DIFF_SUCCESS = 'RECEIVE_FULL_DIFF_SUCCESS';
export const RECEIVE_FULL_DIFF_ERROR = 'RECEIVE_FULL_DIFF_ERROR';
......@@ -6,8 +6,10 @@ import {
addContextLines,
prepareDiffData,
isDiscussionApplicableToLine,
convertExpandLines,
} from './utils';
import * as types from './mutation_types';
import { OLD_LINE_KEY, NEW_LINE_KEY, TYPE_KEY, LEFT_LINE_KEY } from '../constants';
export default {
[types.SET_BASE_CONFIG](state, options) {
......@@ -248,4 +250,54 @@ export default {
[types.TOGGLE_FILE_FINDER_VISIBLE](state, visible) {
state.fileFinderVisible = visible;
},
[types.REQUEST_FULL_DIFF](state, filePath) {
const file = findDiffFile(state.diffFiles, filePath, 'file_path');
file.isLoadingFullFile = true;
},
[types.RECEIVE_FULL_DIFF_ERROR](state, filePath) {
const file = findDiffFile(state.diffFiles, filePath, 'file_path');
file.isLoadingFullFile = false;
},
[types.RECEIVE_FULL_DIFF_SUCCESS](state, { filePath, data }) {
const file = findDiffFile(state.diffFiles, filePath, 'file_path');
file.isShowingFullFile = true;
file.isLoadingFullFile = false;
file.highlighted_diff_lines = convertExpandLines({
diffLines: file.highlighted_diff_lines,
typeKey: [TYPE_KEY],
oldLineKey: [OLD_LINE_KEY],
newLineKey: [NEW_LINE_KEY],
data,
mapLine: ({ line, oldLine, newLine }) => ({
...line,
old_line: oldLine,
new_line: newLine,
line_code: `${file.file_hash}_${oldLine}_${newLine}`,
}),
});
file.parallel_diff_lines = convertExpandLines({
diffLines: file.parallel_diff_lines,
typeKey: [LEFT_LINE_KEY, TYPE_KEY],
oldLineKey: [LEFT_LINE_KEY, OLD_LINE_KEY],
newLineKey: [LEFT_LINE_KEY, NEW_LINE_KEY],
data,
mapLine: ({ line, oldLine, newLine }) => ({
left: {
...line,
old_line: oldLine,
line_code: `${file.file_hash}_${oldLine}_${newLine}`,
},
right: {
...line,
new_line: newLine,
line_code: `${file.file_hash}_${newLine}_${oldLine}`,
},
}),
});
},
};
......@@ -15,8 +15,8 @@ import {
TREE_TYPE,
} from '../constants';
export function findDiffFile(files, hash) {
return files.filter(file => file.file_hash === hash)[0];
export function findDiffFile(files, match, matchKey = 'file_hash') {
return files.find(file => file[matchKey] === match);
}
export const getReversePosition = linePosition => {
......@@ -250,6 +250,8 @@ export function prepareDiffData(diffData) {
renderIt: showingLines < LINES_TO_BE_RENDERED_DIRECTLY,
collapsed:
file.viewer.name === diffViewerModes.text && showingLines > MAX_LINES_TO_BE_RENDERED,
isShowingFullFile: false,
isLoadingFullFile: false,
discussions: [],
});
}
......@@ -411,3 +413,37 @@ export const getDiffMode = diffFile => {
diffModes.replaced
);
};
export const convertExpandLines = ({
diffLines,
data,
typeKey,
oldLineKey,
newLineKey,
mapLine,
}) => {
const dataLength = data.length;
return diffLines.reduce((acc, line, i) => {
if (_.property(typeKey)(line) === 'match') {
const beforeLine = diffLines[i - 1];
const afterLine = diffLines[i + 1];
const beforeLineIndex = _.property(newLineKey)(beforeLine) || 0;
const afterLineIndex = _.property(newLineKey)(afterLine) - 1 || dataLength;
acc.push(
...data.slice(beforeLineIndex, afterLineIndex).map((l, index) => ({
...mapLine({
line: { ...l, hasForm: false, discussions: [] },
oldLine: (_.property(oldLineKey)(beforeLine) || 0) + index + 1,
newLine: (_.property(newLineKey)(beforeLine) || 0) + index + 1,
}),
})),
);
} else {
acc.push(line);
}
return acc;
}, []);
};
......@@ -53,6 +53,10 @@
background-color: $gray-normal;
}
a {
color: $gray-700;
}
svg {
vertical-align: middle;
top: -1px;
......
......@@ -90,65 +90,21 @@ class Projects::BlobController < Projects::ApplicationController
def diff
apply_diff_view_cookie!
@blob.load_all_data!
@lines = @blob.present.highlight.lines
@form = UnfoldForm.new(params.to_unsafe_h)
@lines = @lines[@form.since - 1..@form.to - 1].map(&:html_safe)
if @form.bottom?
@match_line = ''
else
lines_length = @lines.length - 1
line = [@form.since, lines_length].join(',')
@match_line = "@@ -#{line}+#{line} @@"
end
@form = Blobs::UnfoldPresenter.new(blob, params.to_unsafe_h)
# We can keep only 'render_diff_lines' from this conditional when
# keep only json rendering when
# https://gitlab.com/gitlab-org/gitlab-ce/issues/44988 is done
if rendered_for_merge_request?
render_diff_lines
render json: DiffLineSerializer.new.represent(@form.diff_lines)
else
@lines = @form.lines
@match_line = @form.match_line_text
render layout: false
end
end
private
# Converts a String array to Gitlab::Diff::Line array
def render_diff_lines
@lines.map! do |line|
# These are marked as context lines but are loaded from blobs.
# We also have context lines loaded from diffs in other places.
diff_line = Gitlab::Diff::Line.new(line, nil, nil, nil, nil)
diff_line.rich_text = line
diff_line
end
add_match_line
render json: DiffLineSerializer.new.represent(@lines)
end
def add_match_line
return unless @form.unfold?
if @form.bottom? && @form.to < @blob.lines.size
old_pos = @form.to - @form.offset
new_pos = @form.to
elsif @form.since != 1
old_pos = new_pos = @form.since
end
# Match line is not needed when it reaches the top limit or bottom limit of the file.
return unless new_pos
@match_line = Gitlab::Diff::Line.new(@match_line, 'match', nil, old_pos, new_pos)
@form.bottom? ? @lines.push(@match_line) : @lines.unshift(@match_line)
end
def blob
@blob ||= @repository.blob_at(@commit.id, @path)
......@@ -231,6 +187,8 @@ class Projects::BlobController < Projects::ApplicationController
end
def validate_diff_params
return if params[:full]
if [:since, :to, :offset].any? { |key| params[key].blank? }
head :ok
end
......
......@@ -18,6 +18,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
before_action only: [:show] do
push_frontend_feature_flag(:diff_tree_filtering, default_enabled: true)
push_frontend_feature_flag(:expand_diff_full_file)
end
def index
......
# frozen_string_literal: true
require 'gt_one_coercion'
module Blobs
class UnfoldPresenter < BlobPresenter
include Virtus.model
include Gitlab::Utils::StrongMemoize
attribute :full, Boolean, default: false
attribute :since, GtOneCoercion
attribute :to, GtOneCoercion
attribute :bottom, Boolean
attribute :unfold, Boolean, default: true
attribute :offset, Integer
attribute :indent, Integer, default: 0
def initialize(blob, params)
@subject = blob
@all_lines = highlight.lines
super(params)
if full?
self.attributes = { since: 1, to: @all_lines.size, bottom: false, unfold: false, offset: 0, indent: 0 }
end
end
# Converts a String array to Gitlab::Diff::Line array, with match line added
def diff_lines
diff_lines = lines.map do |line|
Gitlab::Diff::Line.new(line, nil, nil, nil, nil, rich_text: line)
end
add_match_line(diff_lines)
diff_lines
end
def lines
strong_memoize(:lines) do
lines = @all_lines
lines = lines[since - 1..to - 1] unless full?
lines.map(&:html_safe)
end
end
def match_line_text
return '' if bottom?
lines_length = lines.length - 1
line = [since, lines_length].join(',')
"@@ -#{line}+#{line} @@"
end
private
def add_match_line(diff_lines)
return unless unfold?
if bottom? && to < @all_lines.size
old_pos = to - offset
new_pos = to
elsif since != 1
old_pos = new_pos = since
end
# Match line is not needed when it reaches the top limit or bottom limit of the file.
return unless new_pos
match_line = Gitlab::Diff::Line.new(match_line_text, 'match', nil, old_pos, new_pos)
bottom? ? diff_lines.push(match_line) : diff_lines.unshift(match_line)
end
end
end
......@@ -7,7 +7,7 @@ class DiffFileEntity < DiffFileBaseEntity
expose :added_lines
expose :removed_lines
expose :load_collapsed_diff_url, if: -> (diff_file, options) { diff_file.viewer.collapsed? && options[:merge_request] } do |diff_file|
expose :load_collapsed_diff_url, if: -> (diff_file, options) { options[:merge_request] } do |diff_file|
merge_request = options[:merge_request]
project = merge_request.target_project
......@@ -57,6 +57,10 @@ class DiffFileEntity < DiffFileBaseEntity
diff_file.diff_lines_for_serializer
end
expose :is_fully_expanded, if: -> (diff_file, _) { Feature.enabled?(:expand_diff_full_file) && diff_file.text? } do |diff_file|
diff_file.fully_expanded?
end
# Used for parallel diffs
expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, _) { diff_file.text? }
end
---
title: Allow expanding a diff to display full file
merge_request: 24406
author:
type: added
......@@ -329,6 +329,16 @@ module Gitlab
lines
end
def fully_expanded?
return true if binary?
lines = diff_lines_for_serializer
return true if lines.nil?
lines.none? { |line| line.type.to_s == 'match' }
end
private
def total_blob_lines(blob)
......
# frozen_string_literal: true
require_relative 'gt_one_coercion'
class UnfoldForm
include Virtus.model
attribute :since, GtOneCoercion
attribute :to, GtOneCoercion
attribute :bottom, Boolean
attribute :unfold, Boolean, default: true
attribute :offset, Integer
attribute :indent, Integer, default: 0
end
......@@ -2944,6 +2944,9 @@ msgstr ""
msgid "Edit environment"
msgstr ""
msgid "Edit file"
msgstr ""
msgid "Edit files in the editor and commit changes here"
msgstr ""
......@@ -4581,6 +4584,12 @@ msgstr ""
msgid "Logs"
msgstr ""
msgid "MRDiff|Show changes only"
msgstr ""
msgid "MRDiff|Show full file"
msgstr ""
msgid "Make sure you're logged into the account that owns the projects you'd like to import."
msgstr ""
......@@ -4749,6 +4758,9 @@ msgstr ""
msgid "MergeRequest| %{paragraphStart}changed the description %{descriptionChangedTimes} times %{timeDifferenceMinutes}%{paragraphEnd}"
msgstr ""
msgid "MergeRequest|Error loading full diff. Please try again."
msgstr ""
msgid "MergeRequest|Filter files"
msgstr ""
......
......@@ -144,54 +144,34 @@ describe Projects::BlobController do
end
context 'when rendering for merge request' do
it 'renders diff context lines Gitlab::Diff::Line array' do
do_get(since: 1, to: 5, offset: 10, from_merge_request: true)
lines = JSON.parse(response.body)
expect(lines.first).to have_key('type')
expect(lines.first).to have_key('rich_text')
expect(lines.first).to have_key('rich_text')
let(:presenter) { double(:presenter, diff_lines: diff_lines) }
let(:diff_lines) do
Array.new(3, Gitlab::Diff::Line.new('plain', nil, nil, nil, nil, rich_text: 'rich'))
end
context 'when rendering match lines' 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)
match_line = JSON.parse(response.body).first
expect(match_line['type']).to eq('match')
expect(match_line['meta_data']).to have_key('old_pos')
expect(match_line['meta_data']).to have_key('new_pos')
end
it 'does not add top match line when "since" is equal 1' do
do_get(since: 1, to: 10, offset: 10, from_merge_request: true)
match_line = JSON.parse(response.body).first
expect(match_line['type']).to be_nil
end
before do
allow(Blobs::UnfoldPresenter).to receive(:new).and_return(presenter)
end
it 'adds bottom match line when "t"o is less than blob size' do
do_get(since: 1, to: 5, offset: 10, from_merge_request: true, bottom: true)
it 'renders diff context lines Gitlab::Diff::Line array' do
do_get(since: 1, to: 2, offset: 0, from_merge_request: true)
match_line = JSON.parse(response.body).last
lines = JSON.parse(response.body)
expect(match_line['type']).to eq('match')
expect(match_line['meta_data']).to have_key('old_pos')
expect(match_line['meta_data']).to have_key('new_pos')
expect(lines.size).to eq(diff_lines.size)
lines.each do |line|
expect(line).to have_key('type')
expect(line['text']).to eq('plain')
expect(line['rich_text']).to eq('rich')
end
end
it 'does not add bottom match line when "to" is less than blob size' do
commit_id = project.repository.commit('master').id
blob = project.repository.blob_at(commit_id, 'CHANGELOG')
do_get(since: 1, to: blob.lines.count, offset: 10, from_merge_request: true, bottom: true)
it 'handles full being true' do
do_get(full: true, from_merge_request: true)
match_line = JSON.parse(response.body).last
lines = JSON.parse(response.body)
expect(match_line['type']).to be_nil
end
expect(lines.size).to eq(diff_lines.size)
end
end
end
......
......@@ -26,7 +26,7 @@ describe 'a maintainer edits files on a source-branch of an MR from a fork', :js
visit project_merge_request_path(target_project, merge_request)
click_link 'Changes'
wait_for_requests
first('.js-file-title').click_link 'Edit'
first('.js-file-title').find('.js-edit-blob').click
wait_for_requests
end
......
......@@ -23,6 +23,9 @@ describe('diff_file_header', () => {
});
beforeEach(() => {
gon.features = {
expandDiffFullFile: true,
};
const diffFile = diffDiscussionMock.diff_file;
diffFile.added_lines = 2;
......@@ -382,7 +385,7 @@ describe('diff_file_header', () => {
props.diffFile.edit_path = '/';
vm = mountComponentWithStore(Component, { props, store });
expect(vm.$el.querySelector('.js-edit-blob')).toContainText('Edit');
expect(vm.$el.querySelector('.js-edit-blob')).not.toBe(null);
});
it('should not show edit button when file is deleted', () => {
......@@ -576,4 +579,66 @@ describe('diff_file_header', () => {
});
});
});
describe('expand full file button', () => {
beforeEach(() => {
props.addMergeRequestButtons = true;
props.diffFile.edit_path = '/';
});
it('does not render button', () => {
vm = mountComponentWithStore(Component, { props, store });
expect(vm.$el.querySelector('.js-expand-file')).toBe(null);
});
it('renders button', () => {
props.diffFile.is_fully_expanded = false;
vm = mountComponentWithStore(Component, { props, store });
expect(vm.$el.querySelector('.js-expand-file')).not.toBe(null);
});
it('shows fully expanded text', () => {
props.diffFile.is_fully_expanded = false;
props.diffFile.isShowingFullFile = true;
vm = mountComponentWithStore(Component, { props, store });
expect(vm.$el.querySelector('.js-expand-file').textContent).toContain('Show changes only');
});
it('shows expand text', () => {
props.diffFile.is_fully_expanded = false;
vm = mountComponentWithStore(Component, { props, store });
expect(vm.$el.querySelector('.js-expand-file').textContent).toContain('Show full file');
});
it('renders loading icon', () => {