From ca0cf5a3cd2829db4cfac007c36d5588ed369f87 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Fri, 30 Dec 2016 17:16:25 -0200 Subject: 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 ++-- .../projects/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 = [ -- cgit v1.2.1