From 90a6be190feea0966e9ed9b6731d930bcff32d68 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 6 Jul 2016 12:36:39 +0100
Subject: [PATCH] Ensure only renderable text diffs are collapsed
Other diffs (those that are too large to render anyway, image diffs,
diffs suppressed by .gitattributes) should be rendered immediately.
---
app/assets/javascripts/single_diff.js.coffee | 10 +-
app/assets/stylesheets/framework/files.scss | 4 +
app/helpers/diff_helper.rb | 1 +
app/views/projects/diffs/_content.html.haml | 6 +-
app/views/projects/diffs/_file.html.haml | 7 +-
.../expand_collapse_diffs_spec.rb | 138 ++++++++++++++++++
.../labels/update_prioritization_spec.rb | 2 +-
spec/support/capybara_helpers.rb | 8 +
8 files changed, 166 insertions(+), 10 deletions(-)
create mode 100644 spec/features/merge_requests/expand_collapse_diffs_spec.rb
diff --git a/app/assets/javascripts/single_diff.js.coffee b/app/assets/javascripts/single_diff.js.coffee
index 4d1c28c082b..76db716317b 100644
--- a/app/assets/javascripts/single_diff.js.coffee
+++ b/app/assets/javascripts/single_diff.js.coffee
@@ -2,13 +2,15 @@ class @SingleDiff
LOADING_HTML = '<i class="fa fa-spinner fa-spin"></i>'
ERROR_HTML = '<div class="nothing-here-block"><i class="fa fa-warning"></i> Could not load diff</div>'
+ COLLAPSED_HTML = '<div class="nothing-here-block diff-collapsed">This diff is collapsed. Click to expand it.</div>'
constructor: (@file) ->
@content = $('.diff-content', @file)
- @diffForPath = @content.data 'diff-for-path'
+ @diffForPath = @content.find('[data-diff-for-path]').data 'diff-for-path'
@setOpenState()
$('.file-title > a', @file).on 'click', @toggleDiff
+ @enableToggleOnContent()
setOpenState: ->
if @diffForPath
@@ -18,11 +20,15 @@ class @SingleDiff
@contentHTML = @content.html()
return
+ enableToggleOnContent: ->
+ @content.find('.nothing-here-block.diff-collapsed').on 'click', @toggleDiff
+
toggleDiff: (e) =>
e.preventDefault()
@isOpen = !@isOpen
if not @isOpen and not @hasError
- @content.empty()
+ @content.html COLLAPSED_HTML
+ @enableToggleOnContent
return
if @contentHTML
@setContentHTML()
diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss
index 71e4b50f2af..02480689f09 100644
--- a/app/assets/stylesheets/framework/files.scss
+++ b/app/assets/stylesheets/framework/files.scss
@@ -189,3 +189,7 @@ span.idiff {
border-bottom-right-radius: 2px;
}
}
+
+.nothing-here-block.diff-collapsed {
+ cursor: pointer;
+}
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index 9a5920edfa2..93f12198a58 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -15,6 +15,7 @@ module DiffHelper
diff_commit = commit_for_diff(diff_file)
blob = project.repository.blob_for_diff(diff_commit, diff_file)
+ @expand_all = true
locals = {
diff_file: diff_file,
diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml
index 832aa0c5a14..9c6a17c0a8c 100644
--- a/app/views/projects/diffs/_content.html.haml
+++ b/app/views/projects/diffs/_content.html.haml
@@ -9,7 +9,11 @@
- if !project.repository.diffable?(blob)
.nothing-here-block This diff was suppressed by a .gitattributes entry.
- elsif diff_file.diff_lines.length > 0
- - if diff_view == 'parallel'
+ - if diff_file.collapsed_by_default? && !@expand_all
+ - url = url_for(params.merge(action: :diff_for_path, path: diff_file.file_path, format: nil))
+ .nothing-here-block.diff-collapsed{data: { diff_for_path: url } }
+ This diff is collapsed. Click to expand it.
+ - elsif diff_view == 'parallel'
= render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob
- else
= render "projects/diffs/text_file", diff_file: diff_file
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index c83ed55efe1..c306909fb1a 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -16,9 +16,4 @@
= view_file_btn(diff_commit.id, diff_file, project)
- - if diff_file.collapsed_by_default?
- - url = url_for(params.merge(action: :diff_for_path, path: diff_file.file_path, format: nil))
- .diff-content.diff-wrap-lines{data: { diff_for_path: url }}
- .nothing-here-block File hidden by default; content for this element available at #{url}
- - else
- = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project
+ = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project
diff --git a/spec/features/merge_requests/expand_collapse_diffs_spec.rb b/spec/features/merge_requests/expand_collapse_diffs_spec.rb
new file mode 100644
index 00000000000..173ea3720da
--- /dev/null
+++ b/spec/features/merge_requests/expand_collapse_diffs_spec.rb
@@ -0,0 +1,138 @@
+require 'spec_helper'
+
+feature 'Expand and collapse diffs', js: true, feature: true do
+ include WaitForAjax
+
+ before do
+ login_as :admin
+ merge_request = create(:merge_request, source_branch: 'expand-collapse-diffs', target_branch: 'master')
+ project = merge_request.source_project
+
+ # Ensure that undiffable.md is in .gitattributes
+ project.repository.copy_gitattributes('expand-collapse-diffs')
+ visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
+ execute_script('window.ajaxUris = []; $(document).ajaxSend(function(event, xhr, settings) { ajaxUris.push(settings.url) });')
+ end
+
+ def file_container(filename)
+ find("[data-blob-diff-path*='#{filename}']")
+ end
+
+ # Use define_method instead of let (which is memoized) so that this just works across a
+ # reload.
+ #
+ ['small_diff.md', 'large_diff.md', 'undiffable.md', 'too_large.md', 'too_large_image.jpg'].each do |file|
+ define_method(file.split('.').first) { file_container(file) }
+ end
+
+ context 'visiting an existing merge request' do
+ it 'shows small diffs immediately' do
+ expect(small_diff).to have_selector('.code')
+ expect(small_diff).not_to have_selector('.nothing-here-block')
+ end
+
+ it 'collapses larges diffs by default' do
+ expect(large_diff).not_to have_selector('.code')
+ expect(large_diff).to have_selector('.nothing-here-block')
+ end
+
+ it 'shows non-renderable diffs as such immediately, regardless of their size' do
+ expect(undiffable).not_to have_selector('.code')
+ expect(undiffable).to have_selector('.nothing-here-block')
+ expect(undiffable).to have_content('gitattributes')
+ end
+
+ it 'does not allow diffs that are larger than the maximum size to be expanded' do
+ expect(too_large).not_to have_selector('.code')
+ expect(too_large).to have_selector('.nothing-here-block')
+ expect(too_large).to have_content('too large')
+ end
+
+ it 'shows image diffs immediately, regardless of their size' do
+ expect(too_large_image).not_to have_selector('.nothing-here-block')
+ expect(too_large_image).to have_selector('.image')
+ end
+
+ context 'expanding a large diff' do
+ before do
+ click_link('large_diff.md')
+ wait_for_ajax
+ end
+
+ it 'makes a request to get the content' do
+ ajax_uris = evaluate_script('ajaxUris')
+
+ expect(ajax_uris).not_to be_empty
+ expect(ajax_uris.first).to include('large_diff.md')
+ end
+
+ it 'shows the diff content' do
+ expect(large_diff).to have_selector('.code')
+ expect(large_diff).not_to have_selector('.nothing-here-block')
+ end
+
+ context 'adding a comment to the expanded diff' do
+ let(:comment_text) { 'A comment' }
+
+ before do
+ large_diff.find('.line_holder', match: :prefer_exact).hover
+ large_diff.find('.add-diff-note').click
+ large_diff.find('.note-textarea').send_keys comment_text
+ large_diff.find_button('Comment').click
+ wait_for_ajax
+ end
+
+ it 'adds the comment' do
+ expect(large_diff.find('.notes')).to have_content comment_text
+ end
+
+ context 'reloading the page' do
+ before { refresh }
+
+ it 'collapses the large diff by default' do
+ expect(large_diff).not_to have_selector('.code')
+ expect(large_diff).to have_selector('.nothing-here-block')
+ end
+
+ context 'expanding the diff' do
+ before do
+ click_link('large_diff.md')
+ wait_for_ajax
+ end
+
+ it 'shows the diff content' do
+ expect(large_diff).to have_selector('.code')
+ expect(large_diff).not_to have_selector('.nothing-here-block')
+ end
+
+ it 'shows the diff comment' do
+ expect(large_diff.find('.notes')).to have_content comment_text
+ end
+ end
+ end
+ end
+ end
+
+ context 'collapsing an expanded diff' do
+ before { click_link('small_diff.md') }
+
+ it 'hides the diff content' do
+ expect(small_diff).not_to have_selector('.code')
+ expect(small_diff).to have_selector('.nothing-here-block')
+ end
+
+ context 're-expanding the same diff' do
+ before { click_link('small_diff.md') }
+
+ it 'shows the diff content' do
+ expect(small_diff).to have_selector('.code')
+ expect(small_diff).not_to have_selector('.nothing-here-block')
+ end
+
+ it 'does not make a new HTTP request' do
+ expect(evaluate_script('ajaxUris')).to be_empty
+ end
+ end
+ end
+ end
+end
diff --git a/spec/features/projects/labels/update_prioritization_spec.rb b/spec/features/projects/labels/update_prioritization_spec.rb
index 6a39c302f55..98ba93b4036 100644
--- a/spec/features/projects/labels/update_prioritization_spec.rb
+++ b/spec/features/projects/labels/update_prioritization_spec.rb
@@ -76,7 +76,7 @@ feature 'Prioritize labels', feature: true do
expect(page.all('li').last).to have_content('bug')
end
- visit current_url
+ refresh
wait_for_ajax
page.within('.prioritized-labels') do
diff --git a/spec/support/capybara_helpers.rb b/spec/support/capybara_helpers.rb
index 9b5c3065eed..b57a3493aff 100644
--- a/spec/support/capybara_helpers.rb
+++ b/spec/support/capybara_helpers.rb
@@ -27,6 +27,14 @@ module CapybaraHelpers
end
end
end
+
+ # Refresh the page. Calling `visit current_url` doesn't seem to work consistently.
+ #
+ def refresh
+ url = current_url
+ visit 'about:blank'
+ visit url
+ end
end
RSpec.configure do |config|
--
GitLab