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.

8 files changed, 166 insertions, 10 deletions

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|