Commit b14de8e1 authored by Phil Hughes's avatar Phil Hughes Committed by Mark Chao

Add option to expand diff to full file

The user can also toggle between the diff changes and
the full file diff.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/19054
parent 2609c2a7
......@@ -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;
......
......@@ -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
......
......@@ -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)
......
......@@ -2929,6 +2929,9 @@ msgstr ""
msgid "Edit environment"
msgstr ""
msgid "Edit file"
msgstr ""
msgid "Edit files in the editor and commit changes here"
msgstr ""
......@@ -4521,6 +4524,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 ""
......@@ -4689,6 +4698,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 ""
......
......@@ -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', () => {
props.diffFile.is_fully_expanded = false;
props.diffFile.isLoadingFullFile = true;
vm = mountComponentWithStore(Component, { props, store });
expect(vm.$el.querySelector('.js-expand-file .loading-container')).not.toBe(null);
});
it('calls toggleFullDiff on click', () => {
props.diffFile.is_fully_expanded = false;
vm = mountComponentWithStore(Component, { props, store });
spyOn(vm.$store, 'dispatch').and.stub();
vm.$el.querySelector('.js-expand-file').click();
expect(vm.$store.dispatch).toHaveBeenCalledWith(
'diffs/toggleFullDiff',
props.diffFile.file_path,
);
});
});
});
......@@ -288,6 +288,7 @@ export default {
external_storage: null,
old_path_html: 'CHANGELOG_OLD',
new_path_html: 'CHANGELOG',
is_fully_expanded: true,
context_lines_path:
'/gitlab-org/gitlab-test/blob/c48ee0d1bf3b30453f5b32250ce03134beaa6d13/CHANGELOG/diff',
highlighted_diff_lines: [
......
......@@ -30,6 +30,11 @@ import actions, {
setRenderTreeList,
setShowWhitespace,
setRenderIt,
requestFullDiff,
receiveFullDiffSucess,
receiveFullDiffError,
fetchFullDiff,
toggleFullDiff,
} from '~/diffs/store/actions';
import eventHub from '~/notes/event_hub';
import * as types from '~/diffs/store/mutation_types';
......@@ -847,4 +852,129 @@ describe('DiffsStoreActions', () => {
testAction(setRenderIt, 'file', {}, [{ type: types.RENDER_FILE, payload: 'file' }], [], done);
});
});
describe('requestFullDiff', () => {
it('commits REQUEST_FULL_DIFF', done => {
testAction(
requestFullDiff,
'file',
{},
[{ type: types.REQUEST_FULL_DIFF, payload: 'file' }],
[],
done,
);
});
});
describe('receiveFullDiffSucess', () => {
it('commits REQUEST_FULL_DIFF', done => {
testAction(
receiveFullDiffSucess,
{ filePath: 'test', data: 'test' },
{},
[{ type: types.RECEIVE_FULL_DIFF_SUCCESS, payload: { filePath: 'test', data: 'test' } }],
[],
done,
);
});
});
describe('receiveFullDiffError', () => {
it('commits REQUEST_FULL_DIFF', done => {
testAction(
receiveFullDiffError,
'file',
{},
[{ type: types.RECEIVE_FULL_DIFF_ERROR, payload: 'file' }],
[],
done,
);
});
});
describe('fetchFullDiff', () => {
let mock;
let scrollToElementSpy;
beforeEach(() => {
scrollToElementSpy = spyOnDependency(actions, 'scrollToElement').and.stub();
mock = new MockAdapter(axios);
});
afterEach(() => {
mock.restore();
});
describe('success', () => {
beforeEach(() => {
mock.onGet(`${gl.TEST_HOST}/context`).replyOnce(200, ['test']);
});
it('dispatches receiveFullDiffSucess', done => {
testAction(
fetchFullDiff,
{ context_lines_path: `${gl.TEST_HOST}/context`, file_path: 'test', file_hash: 'test' },
null,
[],
[{ type: 'receiveFullDiffSucess', payload: { filePath: 'test', data: ['test'] } }],
done,
);
});
it('scrolls to element', done => {
fetchFullDiff(
{ dispatch() {} },
{ context_lines_path: `${gl.TEST_HOST}/context`, file_path: 'test', file_hash: 'test' },
)
.then(() => {
expect(scrollToElementSpy).toHaveBeenCalledWith('#test');
done();
})
.catch(done.fail);
});
});
describe('error', () => {
beforeEach(() => {
mock.onGet(`${gl.TEST_HOST}/context`).replyOnce(500);
});
it('dispatches receiveFullDiffError', done => {
testAction(
fetchFullDiff,
{ context_lines_path: `${gl.TEST_HOST}/context`, file_path: 'test', file_hash: 'test' },
null,
[],
[{ type: 'receiveFullDiffError', payload: 'test' }],
done,
);
});
});
});
describe('toggleFullDiff', () => {
let state;
beforeEach(() => {
state = {
diffFiles: [{ file_path: 'test', isShowingFullFile: false }],
};
});
it('dispatches fetchFullDiff when file is not expanded', done => {
testAction(
toggleFullDiff,
'test',
state,
[],
[
{ type: 'requestFullDiff', payload: 'test' },
{ type: 'fetchFullDiff', payload: state.diffFiles[0] },
],
done,
);
});
});
});
......@@ -680,4 +680,66 @@ describe('DiffsStoreMutations', () => {
expect(state.showWhitespace).toBe(false);
});
});
describe('REQUEST_FULL_DIFF', () => {
it('sets isLoadingFullFile to true', () => {
const state = {
diffFiles: [{ file_path: 'test', isLoadingFullFile: false }],
};
mutations[types.REQUEST_FULL_DIFF](state, 'test');
expect(state.diffFiles[0].isLoadingFullFile).toBe(true);
});
});
describe('RECEIVE_FULL_DIFF_ERROR', () => {
it('sets isLoadingFullFile to false', () => {
const state = {
diffFiles: [{ file_path: 'test', isLoadingFullFile: true }],
};
mutations[types.RECEIVE_FULL_DIFF_ERROR](state, 'test');
expect(state.diffFiles[0].isLoadingFullFile).toBe(false);
});
});
describe('RECEIVE_FULL_DIFF_SUCCESS', () => {
it('sets isLoadingFullFile to false', () => {
const state = {
diffFiles: [
{
file_path: 'test',
isLoadingFullFile: true,
isShowingFullFile: false,
highlighted_diff_lines: [],
parallel_diff_lines: [],
},
],
};
mutations[types.RECEIVE_FULL_DIFF_SUCCESS](state, { filePath: 'test', data: [] });
expect(state.diffFiles[0].isLoadingFullFile).toBe(false);
});
it('sets isShowingFullFile to true', () => {
const state = {
diffFiles: [
{
file_path: 'test',
isLoadingFullFile: true,
isShowingFullFile: false,
highlighted_diff_lines: [],
parallel_diff_lines: [],
},
],
};
mutations[types.RECEIVE_FULL_DIFF_SUCCESS](state, { filePath: 'test', data: [] });
expect(state.diffFiles[0].isShowingFullFile).toBe(true);
});
});
});
......@@ -781,4 +781,49 @@ describe('DiffsStoreUtils', () => {
]);
});
});
describe('convertExpandLines', () => {