diff options
author | Douwe Maan <douwe@gitlab.com> | 2019-03-07 11:33:15 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2019-03-07 11:33:15 +0000 |
commit | 6cfa5ee536c30522241bcf09e805d7eb3adbf481 (patch) | |
tree | 89301502c5cc76bb11cba0ae29cc6df46c97277a | |
parent | c45bb62c0ae36018891a343c7c820fc1a901e33e (diff) | |
parent | cea59dbe030bfde83247ef27c49ffd5267b194ea (diff) | |
download | gitlab-ce-6cfa5ee536c30522241bcf09e805d7eb3adbf481.tar.gz |
Merge branch 'expand-diff-to-full-file' into 'master'
Expand diff to entire file
Closes #19054
See merge request gitlab-org/gitlab-ce!24406
25 files changed, 945 insertions, 155 deletions
diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index 4d33ad23f39..a5125c3d077 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -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" diff --git a/app/assets/javascripts/diffs/components/edit_button.vue b/app/assets/javascripts/diffs/components/edit_button.vue index 803f23b9170..f0cc5de4b33 100644 --- a/app/assets/javascripts/diffs/components/edit_button.vue +++ b/app/assets/javascripts/diffs/components/edit_button.vue @@ -1,5 +1,15 @@ <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> diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 7002655ea49..6f380fe6ece 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -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'; diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index c40775c3259..57ddc923a3e 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -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 () => {}; diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index 71ad108ce88..b441b1de451 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -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'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 5a27388863c..45187d93fef 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -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}`, + }, + }), + }); + }, }; diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 247d1e65fea..27a79369a24 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -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; + }, []); +}; diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 0dbbe9e4c25..e50db5310a6 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -53,6 +53,10 @@ background-color: $gray-normal; } + a { + color: $gray-700; + } + svg { vertical-align: middle; top: -1px; diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 77672e7d9fc..0a33856a8d3 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 46a44841c31..2903f7d705b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -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 diff --git a/app/presenters/blobs/unfold_presenter.rb b/app/presenters/blobs/unfold_presenter.rb new file mode 100644 index 00000000000..7b13db3bb74 --- /dev/null +++ b/app/presenters/blobs/unfold_presenter.rb @@ -0,0 +1,75 @@ +# 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 diff --git a/app/serializers/diff_file_entity.rb b/app/serializers/diff_file_entity.rb index 01ee7af37ed..13711070a46 100644 --- a/app/serializers/diff_file_entity.rb +++ b/app/serializers/diff_file_entity.rb @@ -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 diff --git a/changelogs/unreleased/expand-diff-to-full-file.yml b/changelogs/unreleased/expand-diff-to-full-file.yml new file mode 100644 index 00000000000..f41a6be22e8 --- /dev/null +++ b/changelogs/unreleased/expand-diff-to-full-file.yml @@ -0,0 +1,5 @@ +--- +title: Allow expanding a diff to display full file +merge_request: 24406 +author: +type: added diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index eac9bb88eb6..dbee47a19ee 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -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) diff --git a/lib/unfold_form.rb b/lib/unfold_form.rb deleted file mode 100644 index 05bb3ed7f1c..00000000000 --- a/lib/unfold_form.rb +++ /dev/null @@ -1,14 +0,0 @@ -# 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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9552dd03039..d20ad666ba7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -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 "" diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb index 38957e96798..3801fca09dc 100644 --- a/spec/controllers/projects/blob_controller_spec.rb +++ b/spec/controllers/projects/blob_controller_spec.rb @@ -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 diff --git a/spec/features/merge_request/maintainer_edits_fork_spec.rb b/spec/features/merge_request/maintainer_edits_fork_spec.rb index aead98dae23..b35f985126c 100644 --- a/spec/features/merge_request/maintainer_edits_fork_spec.rb +++ b/spec/features/merge_request/maintainer_edits_fork_spec.rb @@ -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 diff --git a/spec/javascripts/diffs/components/diff_file_header_spec.js b/spec/javascripts/diffs/components/diff_file_header_spec.js index 1201f066d2f..66c5b17b825 100644 --- a/spec/javascripts/diffs/components/diff_file_header_spec.js +++ b/spec/javascripts/diffs/components/diff_file_header_spec.js @@ -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, + ); + }); + }); }); diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 4a091b4580b..fd5dd611383 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -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: [ diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index e47c7906fcb..070bfb2ccd0 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -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, + ); + }); + }); }); diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 09ee691b602..270e7d75666 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -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); + }); + }); }); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 599ea9cd420..1f877910125 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -781,4 +781,49 @@ describe('DiffsStoreUtils', () => { ]); }); }); + + describe('convertExpandLines', () => { + it('converts expanded lines to normal lines', () => { + const diffLines = [ + { + type: 'match', + old_line: 1, + new_line: 1, + }, + { + type: '', + old_line: 2, + new_line: 2, + }, + ]; + + const lines = utils.convertExpandLines({ + diffLines, + data: [{ text: 'expanded' }], + typeKey: 'type', + oldLineKey: 'old_line', + newLineKey: 'new_line', + mapLine: ({ line, oldLine, newLine }) => ({ + ...line, + old_line: oldLine, + new_line: newLine, + }), + }); + + expect(lines).toEqual([ + { + text: 'expanded', + new_line: 1, + old_line: 1, + discussions: [], + hasForm: false, + }, + { + type: '', + old_line: 2, + new_line: 2, + }, + ]); + }); + }); }); diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 862590268ca..611c3e946ed 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -8,6 +8,47 @@ describe Gitlab::Diff::File do let(:diff) { commit.raw_diffs.first } let(:diff_file) { described_class.new(diff, diff_refs: commit.diff_refs, repository: project.repository) } + def create_file(file_name, content) + Files::CreateService.new( + project, + project.owner, + commit_message: 'Update', + start_branch: branch_name, + branch_name: branch_name, + file_path: file_name, + file_content: content + ).execute + + project.commit(branch_name).diffs.diff_files.first + end + + def update_file(file_name, content) + Files::UpdateService.new( + project, + project.owner, + commit_message: 'Update', + start_branch: branch_name, + branch_name: branch_name, + file_path: file_name, + file_content: content + ).execute + + project.commit(branch_name).diffs.diff_files.first + end + + def delete_file(file_name) + Files::DeleteService.new( + project, + project.owner, + commit_message: 'Update', + start_branch: branch_name, + branch_name: branch_name, + file_path: file_name + ).execute + + project.commit(branch_name).diffs.diff_files.first + end + describe '#diff_lines' do let(:diff_lines) { diff_file.diff_lines } @@ -675,47 +716,6 @@ describe Gitlab::Diff::File do end let(:branch_name) { 'master' } - def create_file(file_name, content) - Files::CreateService.new( - project, - project.owner, - commit_message: 'Update', - start_branch: branch_name, - branch_name: branch_name, - file_path: file_name, - file_content: content - ).execute - - project.commit(branch_name).diffs.diff_files.first - end - - def update_file(file_name, content) - Files::UpdateService.new( - project, - project.owner, - commit_message: 'Update', - start_branch: branch_name, - branch_name: branch_name, - file_path: file_name, - file_content: content - ).execute - - project.commit(branch_name).diffs.diff_files.first - end - - def delete_file(file_name) - Files::DeleteService.new( - project, - project.owner, - commit_message: 'Update', - start_branch: branch_name, - branch_name: branch_name, - file_path: file_name - ).execute - - project.commit(branch_name).diffs.diff_files.first - end - context 'when empty file is created' do it 'returns true' do diff_file = create_file('empty.md', '') @@ -751,4 +751,123 @@ describe Gitlab::Diff::File do end end end + + describe '#fully_expanded?' do + let(:project) do + create(:project, :custom_repo, files: {}) + end + let(:branch_name) { 'master' } + + context 'when empty file is created' do + it 'returns true' do + diff_file = create_file('empty.md', '') + + expect(diff_file.fully_expanded?).to be_truthy + end + end + + context 'when empty file is deleted' do + it 'returns true' do + create_file('empty.md', '') + diff_file = delete_file('empty.md') + + expect(diff_file.fully_expanded?).to be_truthy + end + end + + context 'when short file with last line removed' do + it 'returns true' do + create_file('with-content.md', (1..3).to_a.join("\n")) + diff_file = update_file('with-content.md', (1..2).to_a.join("\n")) + + expect(diff_file.fully_expanded?).to be_truthy + end + end + + context 'when a single line is added to empty file' do + it 'returns true' do + create_file('empty.md', '') + diff_file = update_file('empty.md', 'new content') + + expect(diff_file.fully_expanded?).to be_truthy + end + end + + context 'when single line file is changed' do + it 'returns true' do + create_file('file.md', 'foo') + diff_file = update_file('file.md', 'bar') + + expect(diff_file.fully_expanded?).to be_truthy + end + end + + context 'when long file is changed' do + before do + create_file('file.md', (1..999).to_a.join("\n")) + end + + context 'when first line is removed' do + it 'returns true' do + diff_file = update_file('file.md', (2..999).to_a.join("\n")) + + expect(diff_file.fully_expanded?).to be_falsey + end + end + + context 'when last line is removed' do + it 'returns true' do + diff_file = update_file('file.md', (1..998).to_a.join("\n")) + + expect(diff_file.fully_expanded?).to be_falsey + end + end + + context 'when first and last lines are removed' do + it 'returns false' do + diff_file = update_file('file.md', (2..998).to_a.join("\n")) + + expect(diff_file.fully_expanded?).to be_falsey + end + end + + context 'when first and last lines are changed' do + it 'returns false' do + content = (2..998).to_a + content.prepend('a') + content.append('z') + content = content.join("\n") + + diff_file = update_file('file.md', content) + + expect(diff_file.fully_expanded?).to be_falsey + end + end + + context 'when every line are changed' do + it 'returns true' do + diff_file = update_file('file.md', "hi\n" * 999) + + expect(diff_file.fully_expanded?).to be_truthy + end + end + + context 'when all contents are cleared' do + it 'returns true' do + diff_file = update_file('file.md', "") + + expect(diff_file.fully_expanded?).to be_truthy + end + end + + context 'when file is binary' do + it 'returns true' do + diff_file = update_file('file.md', (1..998).to_a.join("\n")) + allow(diff_file).to receive(:binary?).and_return(true) + + expect(diff_file.fully_expanded?).to be_truthy + end + end + end + end end diff --git a/spec/presenters/blobs/unfold_presenter_spec.rb b/spec/presenters/blobs/unfold_presenter_spec.rb new file mode 100644 index 00000000000..7ece5f623ce --- /dev/null +++ b/spec/presenters/blobs/unfold_presenter_spec.rb @@ -0,0 +1,159 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Blobs::UnfoldPresenter do + include FakeBlobHelpers + + let(:project) { create(:project, :repository) } + let(:blob) { fake_blob(path: 'foo', data: "1\n2\n3") } + let(:subject) { described_class.new(blob, params) } + + describe '#initialize' do + context 'when full is false' do + let(:params) { { full: false, since: 2, to: 3, bottom: false, offset: 1, indent: 1 } } + + it 'sets attributes' do + result = subject + + expect(result.full?).to eq(false) + expect(result.since).to eq(2) + expect(result.to).to eq(3) + expect(result.bottom).to eq(false) + expect(result.offset).to eq(1) + expect(result.indent).to eq(1) + end + end + + context 'when full is true' do + let(:params) { { full: true, since: 2, to: 3, bottom: false, offset: 1, indent: 1 } } + + it 'sets other attributes' do + result = subject + + expect(result.full?).to eq(true) + expect(result.since).to eq(1) + expect(result.to).to eq(blob.lines.size) + expect(result.bottom).to eq(false) + expect(result.offset).to eq(0) + expect(result.indent).to eq(0) + end + end + end + + describe '#diff_lines' do + let(:total_lines) { 50 } + let(:blob) { fake_blob(path: 'foo', data: (1..total_lines).to_a.join("\n")) } + + context 'when "full" is true' do + let(:params) { { full: true } } + + it 'returns all lines' do + lines = subject.diff_lines + + expect(lines.size).to eq(total_lines) + + lines.each.with_index do |line, index| + expect(line.text).to include("LC#{index + 1}") + expect(line.text).to eq(line.rich_text) + expect(line.type).to be_nil + end + end + + context 'when last line is empty' do + let(:blob) { fake_blob(path: 'foo', data: "1\n2\n") } + + it 'disregards last line' do + lines = subject.diff_lines + + expect(lines.size).to eq(2) + end + end + end + + context 'when "since" is equal to 1' do + let(:params) { { since: 1, to: 10, offset: 10 } } + + it 'does not add top match line' do + line = subject.diff_lines.first + + expect(line.type).to be_nil + end + end + + context 'when since is greater than 1' do + let(:params) { { since: 5, to: 10, offset: 10 } } + + it 'adds top match line' do + line = subject.diff_lines.first + + expect(line.type).to eq('match') + expect(line.old_pos).to eq(5) + expect(line.new_pos).to eq(5) + end + end + + context 'when "to" is less than blob size' do + let(:params) { { since: 1, to: 5, offset: 10, bottom: true } } + + it 'adds bottom match line' do + line = subject.diff_lines.last + + expect(line.type).to eq('match') + expect(line.old_pos).to eq(-5) + expect(line.new_pos).to eq(5) + end + end + + context 'when "to" is equal to blob size' do + let(:params) { { since: 1, to: total_lines, offset: 10, bottom: true } } + + it 'does not add bottom match line' do + line = subject.diff_lines.last + + expect(line.type).to be_nil + end + end + end + + describe '#lines' do + context 'when scope is specified' do + let(:params) { { since: 2, to: 3 } } + + it 'returns lines cropped by params' do + expect(subject.lines.size).to eq(2) + expect(subject.lines[0]).to include('LC2') + expect(subject.lines[1]).to include('LC3') + end + end + + context 'when full is true' do + let(:params) { { full: true } } + + it 'returns all lines' do + expect(subject.lines.size).to eq(3) + expect(subject.lines[0]).to include('LC1') + expect(subject.lines[1]).to include('LC2') + expect(subject.lines[2]).to include('LC3') + end + end + end + + describe '#match_line_text' do + context 'when bottom is true' do + let(:params) { { since: 2, to: 3, bottom: true } } + + it 'returns empty string' do + expect(subject.match_line_text).to eq('') + end + end + + context 'when bottom is false' do + let(:params) { { since: 2, to: 3, bottom: false } } + + it 'returns match line string' do + expect(subject.match_line_text).to eq("@@ -2,1+2,1 @@") + end + end + end +end |