From ca0cf5a3cd2829db4cfac007c36d5588ed369f87 Mon Sep 17 00:00:00 2001
From: Felipe Artur <felipefac@gmail.com>
Date: Fri, 30 Dec 2016 17:16:25 -0200
Subject: [PATCH] Show 'too many changes' message for merge request
---
app/helpers/diff_helper.rb | 6 +++++
app/models/merge_request_diff.rb | 20 +++++++-------
app/views/devise/shared/_signup_box.html.haml | 2 +-
app/views/projects/diffs/_diffs.html.haml | 4 +--
.../merge_requests/show/_diffs.html.haml | 10 +------
changelogs/unreleased/issue_25017.yml | 4 +++
spec/features/merge_requests/diffs_spec.rb | 14 ++++++++++
spec/models/merge_request_diff_spec.rb | 26 +++++++++++++++++++
8 files changed, 64 insertions(+), 22 deletions(-)
create mode 100644 changelogs/unreleased/issue_25017.yml
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index c35d6611ab0..aed1d7c839f 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -165,4 +165,10 @@ module DiffHelper
link_to "#{hide_whitespace? ? 'Show' : 'Hide'} whitespace changes", url, class: options[:class]
end
+
+ def render_overflow_warning?(diff_files)
+ diffs = @merge_request_diff.presence || diff_files
+
+ diffs.overflow?
+ end
end
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index b8f36a2c958..f0e2fadc32b 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -234,28 +234,28 @@ class MergeRequestDiff < ActiveRecord::Base
# and save it as array of hashes in st_diffs db field
def save_diffs
new_attributes = {}
- new_diffs = []
if commits.size.zero?
new_attributes[:state] = :empty
else
diff_collection = compare.diffs(Commit.max_diff_options)
-
- if diff_collection.overflow?
- # Set our state to 'overflow' to make the #empty? and #collected?
- # methods (generated by StateMachine) return false.
- new_attributes[:state] = :overflow
- end
-
- new_attributes[:real_size] = diff_collection.real_size
+ new_attributes[:real_size] = compare.diffs.real_size
if diff_collection.any?
new_diffs = dump_diffs(diff_collection)
new_attributes[:state] = :collected
end
+
+ new_attributes[:st_diffs] = new_diffs || []
+
+ # Set our state to 'overflow' to make the #empty? and #collected?
+ # methods (generated by StateMachine) return false.
+ #
+ # This attribution has to come at the end of the method so 'overflow'
+ # state does not get overridden by 'collected'.
+ new_attributes[:state] = :overflow if diff_collection.overflow?
end
- new_attributes[:st_diffs] = new_diffs
update_columns_serialized(new_attributes)
end
diff --git a/app/views/devise/shared/_signup_box.html.haml b/app/views/devise/shared/_signup_box.html.haml
index 681eb303b49..01ecf237925 100644
--- a/app/views/devise/shared/_signup_box.html.haml
+++ b/app/views/devise/shared/_signup_box.html.haml
@@ -15,7 +15,7 @@
.form-group
= f.label :email
= f.email_field :email, class: "form-control middle", required: true, title: "Please provide a valid email address."
- %div.form-group
+ .form-group
= f.label :email_confirmation
= f.email_field :email_confirmation, class: "form-control middle", required: true, title: "Please retype the email address."
.form-group.append-bottom-20#password-strength
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index ab4a2dc36e5..58c20e225c6 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -18,8 +18,8 @@
= parallel_diff_btn
= render 'projects/diffs/stats', diff_files: diff_files
-- if diff_files.overflow?
- = render 'projects/diffs/warning', diff_files: diff_files
+- if render_overflow_warning?(diff_files)
+ = render 'projects/diffs/warning', diff_files: diffs
.files{ data: { can_create_note: can_create_note } }
- diff_files.each_with_index do |diff_file|
diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml
index 99c71e1454a..5f048d04b27 100644
--- a/app/views/projects/merge_requests/show/_diffs.html.haml
+++ b/app/views/projects/merge_requests/show/_diffs.html.haml
@@ -1,13 +1,5 @@
-- if @merge_request_diff.collected?
+- if @merge_request_diff.collected? || @merge_request_diff.overflow?
= render 'projects/merge_requests/show/versions'
= render "projects/diffs/diffs", diffs: @diffs
- elsif @merge_request_diff.empty?
.nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch}
-- else
- .alert.alert-warning
- %h4
- Changes view for this comparison is extremely large.
- %p
- You can
- = link_to "download it", merge_request_path(@merge_request, format: :diff), class: "vlink"
- instead.
diff --git a/changelogs/unreleased/issue_25017.yml b/changelogs/unreleased/issue_25017.yml
new file mode 100644
index 00000000000..09126ae81bc
--- /dev/null
+++ b/changelogs/unreleased/issue_25017.yml
@@ -0,0 +1,4 @@
+---
+title: Show 'too many changes' message for created merge requests when they are too large
+merge_request:
+author:
diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb
index c9a0059645d..4a6c76a5caf 100644
--- a/spec/features/merge_requests/diffs_spec.rb
+++ b/spec/features/merge_requests/diffs_spec.rb
@@ -22,4 +22,18 @@ feature 'Diffs URL', js: true, feature: true do
expect(page).to have_css('.diffs.tab-pane.active')
end
end
+
+ context 'when merge request has overflow' do
+ it 'displays warning' do
+ allow_any_instance_of(MergeRequestDiff).to receive(:overflow?).and_return(true)
+ allow(Commit).to receive(:max_diff_options).and_return(max_files: 20, max_lines: 20)
+
+ visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)
+
+ page.within('.alert') do
+ expect(page).to have_text("Too many changes to show. Plain diff Email patch To preserve
+ performance only 3 of 3+ files are displayed.")
+ end
+ end
+ end
end
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index eb876d105da..6d599e148a2 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -76,6 +76,32 @@ describe MergeRequestDiff, models: true do
end
end
+ describe '#save_diffs' do
+ it 'saves collected state' do
+ mr_diff = create(:merge_request).merge_request_diff
+
+ expect(mr_diff.collected?).to be_truthy
+ end
+
+ it 'saves overflow state' do
+ allow(Commit).to receive(:max_diff_options)
+ .and_return(max_lines: 0, max_files: 0)
+
+ mr_diff = create(:merge_request).merge_request_diff
+
+ expect(mr_diff.overflow?).to be_truthy
+ end
+
+ it 'saves empty state' do
+ allow_any_instance_of(MergeRequestDiff).to receive(:commits)
+ .and_return([])
+
+ mr_diff = create(:merge_request).merge_request_diff
+
+ expect(mr_diff.empty?).to be_truthy
+ end
+ end
+
describe '#commits_sha' do
it 'returns all commits SHA using serialized commits' do
subject.st_commits = [
--
GitLab