From b6b26692ea44cfeab7e8fd64b7df60852850fce2 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 28 Jun 2016 17:25:32 +0100 Subject: Collapse large diffs by default When rendering a list of diff files, skip those where the diff is over 10 KB and provide an endpoint to render individually instead. --- CHANGELOG | 2 + app/controllers/projects/commit_controller.rb | 20 +- app/controllers/projects/compare_controller.rb | 18 +- .../projects/merge_requests_controller.rb | 76 ++++-- app/helpers/diff_helper.rb | 19 ++ app/models/merge_request.rb | 6 +- app/models/merge_request_diff.rb | 6 + app/views/projects/diffs/_content.html.haml | 25 ++ app/views/projects/diffs/_file.html.haml | 31 +-- config/routes.rb | 15 +- lib/gitlab/diff/file.rb | 4 + spec/controllers/commit_controller_spec.rb | 246 ----------------- .../controllers/projects/commit_controller_spec.rb | 291 ++++++++++++++++++++- .../projects/compare_controller_spec.rb | 69 +++++ .../projects/merge_requests_controller_spec.rb | 236 ++++++++++++----- spec/models/merge_request_diff_spec.rb | 47 ++++ spec/models/merge_request_spec.rb | 25 ++ 17 files changed, 762 insertions(+), 374 deletions(-) create mode 100644 app/views/projects/diffs/_content.html.haml delete mode 100644 spec/controllers/commit_controller_spec.rb create mode 100644 spec/models/merge_request_diff_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 09f2c44e02c..680078622cf 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -32,6 +32,8 @@ v 8.10.0 (unreleased) - Fix user creation with stronger minimum password requirements !4054 (nathan-pmt) - PipelinesFinder uses git cache data - Throttle the update of `project.pushes_since_gc` to 1 minute. + - Allow expanding and collapsing files in diff view (!4990) + - Collapse large diffs by default (!4990) - Check for conflicts with existing Project's wiki path when creating a new project. - Show last push widget in upstream after push to fork - Don't instantiate a git tree on Projects show default view diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 37d6521026c..810653b4264 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -19,7 +19,7 @@ class Projects::CommitController < Projects::ApplicationController @grouped_diff_notes = commit.notes.grouped_diff_notes @notes = commit.notes.non_diff_notes.fresh - + Banzai::NoteRenderer.render( @grouped_diff_notes.values.flatten + @notes, @project, @@ -41,6 +41,24 @@ class Projects::CommitController < Projects::ApplicationController end end + def diff_for_path + return git_not_found! unless commit + + opts = diff_options + opts[:ignore_whitespace_change] = true if params[:format] == 'diff' + + diffs = commit.diffs(opts.merge(paths: [params[:path]])) + diff_refs = [commit.parent || commit, commit] + + @comments_target = { + noteable_type: 'Commit', + commit_id: @commit.id + } + @grouped_diff_notes = {} + + render_diff_for_path(diffs, diff_refs, @project) + end + def builds end diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index d240b9fe989..8a04f63f4d4 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -6,7 +6,7 @@ class Projects::CompareController < Projects::ApplicationController # Authorize before_action :require_non_empty_project before_action :authorize_download_code! - before_action :assign_ref_vars, only: [:index, :show] + before_action :assign_ref_vars, only: [:index, :show, :diff_for_path] before_action :merge_request, only: [:index, :show] def index @@ -35,6 +35,22 @@ class Projects::CompareController < Projects::ApplicationController end end + def diff_for_path + compare = CompareService.new. + execute(@project, @head_ref, @project, @base_ref, diff_options) + + return render_404 unless compare + + @commit = @project.commit(@head_ref) + @base_commit = @project.merge_base_commit(@base_ref, @head_ref) + diffs = compare.diffs(diff_options.merge(paths: [params[:path]])) + + @diff_notes_disabled = true + @grouped_diff_notes = {} + + render_diff_for_path(diffs, [@base_commit, @commit], @project) + end + def create redirect_to namespace_project_compare_path(@project.namespace, @project, params[:from], params[:to]) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index df1943dd9bb..3fc5a319c9b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -13,6 +13,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds] before_action :define_show_vars, only: [:show, :diffs, :commits, :builds] before_action :define_widget_vars, only: [:merge, :cancel_merge_when_build_succeeds, :merge_check] + before_action :define_commit_vars, only: [:diffs] + before_action :define_diff_comment_vars, only: [:diffs] before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds] # Allow read any merge_request @@ -58,7 +60,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController respond_to do |format| format.html - + format.json do render json: @merge_request end @@ -80,32 +82,32 @@ class Projects::MergeRequestsController < Projects::ApplicationController def diffs apply_diff_view_cookie! - @commit = @merge_request.diff_head_commit - @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit - - @comments_target = { - noteable_type: 'MergeRequest', - noteable_id: @merge_request.id - } - - @use_legacy_diff_notes = !@merge_request.support_new_diff_notes? - @grouped_diff_notes = @merge_request.notes.grouped_diff_notes - - Banzai::NoteRenderer.render( - @grouped_diff_notes.values.flatten, - @project, - current_user, - @path, - @project_wiki, - @ref - ) - respond_to do |format| format.html format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } } end end + # With an ID param, loads the MR at that ID. Otherwise, accepts the same params as #new + # and uses that (unsaved) MR. + # + def diff_for_path + if params[:id] + merge_request + define_diff_comment_vars + else + build_merge_request + @diff_notes_disabled = true + @grouped_diff_notes = {} + end + + define_commit_vars + diffs = @merge_request.diffs(diff_options.merge(paths: [params[:path]])) + diff_refs = @merge_request.diff_refs + + render_diff_for_path(diffs, diff_refs, @merge_request.project) + end + def commits respond_to do |format| format.html { render 'show' } @@ -121,8 +123,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def new - params[:merge_request] ||= ActionController::Parameters.new(source_project: @project) - @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute + build_merge_request @noteable = @merge_request @target_branches = if @merge_request.target_project @@ -380,6 +381,30 @@ class Projects::MergeRequestsController < Projects::ApplicationController closes_issues end + def define_commit_vars + @commit = @merge_request.diff_head_commit + @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit + end + + def define_diff_comment_vars + @comments_target = { + noteable_type: 'MergeRequest', + noteable_id: @merge_request.id + } + + @use_legacy_diff_notes = !@merge_request.support_new_diff_notes? + @grouped_diff_notes = @merge_request.notes.grouped_diff_notes + + Banzai::NoteRenderer.render( + @grouped_diff_notes.values.flatten, + @project, + current_user, + @path, + @project_wiki, + @ref + ) + end + def invalid_mr # Render special view for MR with removed source or target branch render 'invalid' @@ -408,4 +433,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController params[:merge_when_build_succeeds].present? && @merge_request.pipeline && @merge_request.pipeline.active? end + + def build_merge_request + params[:merge_request] ||= ActionController::Parameters.new(source_project: @project) + @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute + end end diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index eb57516247d..d4655d60799 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -8,6 +8,25 @@ module DiffHelper [marked_old_line, marked_new_line] end + def render_diff_for_path(diffs, diff_refs, project) + diff_file = safe_diff_files(diffs, diff_refs).first + + return render_404 unless diff_file + + diff_commit = commit_for_diff(diff_file) + blob = project.repository.blob_for_diff(diff_commit, diff_file) + + locals = { + diff_file: diff_file, + diff_commit: diff_commit, + diff_refs: diff_refs, + blob: blob, + project: project + } + + render json: { html: view_to_html_string('projects/diffs/_content', locals) } + end + def diff_view diff_views = %w(inline parallel) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 083e93f1ee7..d5c23716b04 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -19,7 +19,7 @@ class MergeRequest < ActiveRecord::Base after_create :create_merge_request_diff, unless: :importing? after_update :update_merge_request_diff - delegate :commits, :diffs, :real_size, to: :merge_request_diff, prefix: nil + delegate :commits, :real_size, to: :merge_request_diff, prefix: nil # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests @@ -164,6 +164,10 @@ class MergeRequest < ActiveRecord::Base merge_request_diff ? merge_request_diff.first_commit : compare_commits.first end + def diffs(*args) + merge_request_diff ? merge_request_diff.diffs(*args) : compare.diffs(*args) + end + def diff_size merge_request_diff.size end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index ba235750aeb..06b28fc5a75 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -144,6 +144,12 @@ class MergeRequestDiff < ActiveRecord::Base def load_diffs(raw, options) if raw.respond_to?(:each) + if options[:paths] + raw = raw.select do |diff| + options[:paths].include?(diff[:new_path]) + end + end + Gitlab::Git::DiffCollection.new(raw, options) else Gitlab::Git::DiffCollection.new([]) diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml new file mode 100644 index 00000000000..832aa0c5a14 --- /dev/null +++ b/app/views/projects/diffs/_content.html.haml @@ -0,0 +1,25 @@ +.diff-content.diff-wrap-lines + - # Skip all non non-supported blobs + - return unless blob.respond_to?(:text?) + - if diff_file.too_large? + .nothing-here-block This diff could not be displayed because it is too large. + - elsif blob.only_display_raw? + .nothing-here-block This file is too large to display. + - elsif blob_text_viewable?(blob) + - 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' + = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob + - else + = render "projects/diffs/text_file", diff_file: diff_file + - else + - if diff_file.mode_changed? + .nothing-here-block File mode changed + - elsif diff_file.renamed_file + .nothing-here-block File moved + - elsif blob.image? + - old_blob = diff_file.old_blob(diff_commit) + = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob + - else + .nothing-here-block No preview for this file type diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index 3b758a1ec4e..c83ed55efe1 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -16,28 +16,9 @@ = view_file_btn(diff_commit.id, diff_file, project) - .diff-content.diff-wrap-lines - - # Skip all non non-supported blobs - - return unless blob.respond_to?(:text?) - - if diff_file.too_large? - .nothing-here-block This diff could not be displayed because it is too large. - - elsif blob.only_display_raw? - .nothing-here-block This file is too large to display. - - elsif blob_text_viewable?(blob) - - 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' - = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i - - else - = render "projects/diffs/text_file", diff_file: diff_file, index: i - - else - - if diff_file.mode_changed? - .nothing-here-block File mode changed - - elsif diff_file.renamed_file - .nothing-here-block File moved - - elsif blob.image? - - old_blob = diff_file.old_blob(diff_commit) - = render "projects/diffs/image", diff_file: diff_file, old_file: old_blob, file: blob, index: i - - else - .nothing-here-block No preview for this file type + - 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 diff --git a/config/routes.rb b/config/routes.rb index 18a4ead2b37..f31f8171993 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -615,10 +615,18 @@ Rails.application.routes.draw do post :retry_builds post :revert post :cherry_pick + get '/diffs/*path', action: :diff_for_path, constraints: { format: false } end end - resources :compare, only: [:index, :create] + resources :compare, only: [:index, :create] do + collection do + get '/diffs/*path', action: :diff_for_path, constraints: { format: false } + end + end + + get '/compare/:from...:to', to: 'compare#show', as: 'compare', constraints: { from: /.+/, to: /.+/ } + resources :network, only: [:show], constraints: { id: /(?:[^.]|\.(?!json$))+/, format: /json/ } resources :graphs, only: [:show], constraints: { id: /(?:[^.]|\.(?!json$))+/, format: /json/ } do @@ -629,9 +637,6 @@ Rails.application.routes.draw do end end - get '/compare/:from...:to' => 'compare#show', :as => 'compare', - :constraints => { from: /.+/, to: /.+/ } - resources :snippets, constraints: { id: /\d+/ } do member do get 'raw' @@ -706,12 +711,14 @@ Rails.application.routes.draw do post :toggle_subscription post :toggle_award_emoji post :remove_wip + get '/diffs/*path', action: :diff_for_path, constraints: { format: false } end collection do get :branch_from get :branch_to get :update_branches + get '/diffs/*path', action: :diff_for_path, constraints: { format: false } end end diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index b0c50edba59..7e01f7b61fb 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -68,6 +68,10 @@ module Gitlab @lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a end + def collapsed_by_default? + diff.diff.bytesize > 10240 # 10 KB + end + def highlighted_diff_lines @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight end diff --git a/spec/controllers/commit_controller_spec.rb b/spec/controllers/commit_controller_spec.rb deleted file mode 100644 index a3a3309e15e..00000000000 --- a/spec/controllers/commit_controller_spec.rb +++ /dev/null @@ -1,246 +0,0 @@ -require 'spec_helper' - -describe Projects::CommitController do - let(:project) { create(:project) } - let(:user) { create(:user) } - let(:commit) { project.commit("master") } - let(:master_pickable_sha) { '7d3b0f7cff5f37573aea97cebfd5692ea1689924' } - let(:master_pickable_commit) { project.commit(master_pickable_sha) } - - before do - sign_in(user) - project.team << [user, :master] - end - - describe "#show" do - shared_examples "export as" do |format| - it "should generally work" do - get(:show, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id, - format: format) - - expect(response).to be_success - end - - it "should generate it" do - expect_any_instance_of(Commit).to receive(:"to_#{format}") - - get(:show, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id, format: format) - end - - it "should render it" do - get(:show, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id, format: format) - - expect(response.body).to eq(commit.send(:"to_#{format}")) - end - - it "should not escape Html" do - allow_any_instance_of(Commit).to receive(:"to_#{format}"). - and_return('HTML entities &<>" ') - - get(:show, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id, format: format) - - expect(response.body).not_to include('&') - expect(response.body).not_to include('>') - expect(response.body).not_to include('<') - expect(response.body).not_to include('"') - end - end - - describe "as diff" do - include_examples "export as", :diff - let(:format) { :diff } - - it "should really only be a git diff" do - get(:show, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id, - format: format) - - expect(response.body).to start_with("diff --git") - end - - it "should really only be a git diff without whitespace changes" do - get(:show, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: '66eceea0db202bb39c4e445e8ca28689645366c5', - # id: commit.id, - format: format, - w: 1) - - expect(response.body).to start_with("diff --git") - # without whitespace option, there are more than 2 diff_splits - diff_splits = assigns(:diffs).first.diff.split("\n") - expect(diff_splits.length).to be <= 2 - end - end - - describe "as patch" do - include_examples "export as", :patch - let(:format) { :patch } - - it "should really be a git email patch" do - get(:show, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id, - format: format) - - expect(response.body).to start_with("From #{commit.id}") - end - - it "should contain a git diff" do - get(:show, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id, - format: format) - - expect(response.body).to match(/^diff --git/) - end - end - - context 'commit that removes a submodule' do - render_views - - let(:fork_project) { create(:forked_project_with_submodules) } - let(:commit) { fork_project.commit('remove-submodule') } - - before do - fork_project.team << [user, :master] - end - - it 'renders it' do - get(:show, - namespace_id: fork_project.namespace.to_param, - project_id: fork_project.to_param, - id: commit.id) - - expect(response).to be_success - end - end - end - - describe "#branches" do - it "contains branch and tags information" do - get(:branches, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id) - - expect(assigns(:branches)).to include("master", "feature_conflict") - expect(assigns(:tags)).to include("v1.1.0") - end - end - - describe '#revert' do - context 'when target branch is not provided' do - it 'should render the 404 page' do - post(:revert, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: commit.id) - - expect(response).not_to be_success - expect(response).to have_http_status(404) - end - end - - context 'when the revert was successful' do - it 'should redirect to the commits page' do - post(:revert, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', - id: commit.id) - - expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master') - expect(flash[:notice]).to eq('The commit has been successfully reverted.') - end - end - - context 'when the revert failed' do - before do - post(:revert, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', - id: commit.id) - end - - it 'should redirect to the commit page' do - # Reverting a commit that has been already reverted. - post(:revert, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', - id: commit.id) - - expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id) - expect(flash[:alert]).to match('Sorry, we cannot revert this commit automatically.') - end - end - end - - describe '#cherry_pick' do - context 'when target branch is not provided' do - it 'should render the 404 page' do - post(:cherry_pick, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: master_pickable_commit.id) - - expect(response).not_to be_success - expect(response).to have_http_status(404) - end - end - - context 'when the cherry-pick was successful' do - it 'should redirect to the commits page' do - post(:cherry_pick, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', - id: master_pickable_commit.id) - - expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master') - expect(flash[:notice]).to eq('The commit has been successfully cherry-picked.') - end - end - - context 'when the cherry_pick failed' do - before do - post(:cherry_pick, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', - id: master_pickable_commit.id) - end - - it 'should redirect to the commit page' do - # Cherry-picking a commit that has been already cherry-picked. - post(:cherry_pick, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - target_branch: 'master', - id: master_pickable_commit.id) - - expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, master_pickable_commit.id) - expect(flash[:alert]).to match('Sorry, we cannot cherry-pick this commit automatically.') - end - end - end -end diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index 6e3db10e451..9679b4f849f 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -1,9 +1,29 @@ -require 'rails_helper' +require 'spec_helper' describe Projects::CommitController do - describe 'GET show' do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:commit) { project.commit("master") } + let(:master_pickable_sha) { '7d3b0f7cff5f37573aea97cebfd5692ea1689924' } + let(:master_pickable_commit) { project.commit(master_pickable_sha) } + + before do + sign_in(user) + project.team << [user, :master] + end + + describe 'GET #show' do render_views + def go(extra_params = {}) + params = { + namespace_id: project.namespace.to_param, + project_id: project.to_param + } + + get :show, params.merge(extra_params) + end + let(:project) { create(:project) } before do @@ -15,7 +35,7 @@ describe Projects::CommitController do context 'with valid id' do it 'responds with 200' do - go id: project.commit.id + go(id: commit.id) expect(response).to be_ok end @@ -23,27 +43,274 @@ describe Projects::CommitController do context 'with invalid id' do it 'responds with 404' do - go id: project.commit.id.reverse + go(id: commit.id.reverse) expect(response).to be_not_found end end it 'handles binary files' do - get(:show, + go(id: TestEnv::BRANCH_SHA['binary-encoding'], format: 'html') + + expect(response).to be_success + end + + shared_examples "export as" do |format| + it "should generally work" do + go(id: commit.id, format: format) + + expect(response).to be_success + end + + it "should generate it" do + expect_any_instance_of(Commit).to receive(:"to_#{format}") + + go(id: commit.id, format: format) + end + + it "should render it" do + go(id: commit.id, format: format) + + expect(response.body).to eq(commit.send(:"to_#{format}")) + end + + it "should not escape Html" do + allow_any_instance_of(Commit).to receive(:"to_#{format}"). + and_return('HTML entities &<>" ') + + go(id: commit.id, format: format) + + expect(response.body).not_to include('&') + expect(response.body).not_to include('>') + expect(response.body).not_to include('<') + expect(response.body).not_to include('"') + end + end + + describe "as diff" do + include_examples "export as", :diff + let(:format) { :diff } + + it "should really only be a git diff" do + go(id: commit.id, format: format) + + expect(response.body).to start_with("diff --git") + end + + it "should really only be a git diff without whitespace changes" do + go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format, w: 1) + + expect(response.body).to start_with("diff --git") + # without whitespace option, there are more than 2 diff_splits + diff_splits = assigns(:diffs).first.diff.split("\n") + expect(diff_splits.length).to be <= 2 + end + end + + describe "as patch" do + include_examples "export as", :patch + let(:format) { :patch } + + it "should really be a git email patch" do + go(id: commit.id, format: format) + + expect(response.body).to start_with("From #{commit.id}") + end + + it "should contain a git diff" do + go(id: commit.id, format: format) + + expect(response.body).to match(/^diff --git/) + end + end + + context 'commit that removes a submodule' do + render_views + + let(:fork_project) { create(:forked_project_with_submodules, visibility_level: 20) } + let(:commit) { fork_project.commit('remove-submodule') } + + it 'renders it' do + get(:show, + namespace_id: fork_project.namespace.to_param, + project_id: fork_project.to_param, + id: commit.id) + + expect(response).to be_success + end + end + end + + describe "#branches" do + it "contains branch and tags information" do + get(:branches, namespace_id: project.namespace.to_param, project_id: project.to_param, - id: TestEnv::BRANCH_SHA['binary-encoding'], - format: "html") + id: commit.id) - expect(response).to be_success + expect(assigns(:branches)).to include("master", "feature_conflict") + expect(assigns(:tags)).to include("v1.1.0") + end + end + + describe '#revert' do + context 'when target branch is not provided' do + it 'should render the 404 page' do + post(:revert, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: commit.id) + + expect(response).not_to be_success + expect(response).to have_http_status(404) + end + end + + context 'when the revert was successful' do + it 'should redirect to the commits page' do + post(:revert, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: commit.id) + + expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master') + expect(flash[:notice]).to eq('The commit has been successfully reverted.') + end + end + + context 'when the revert failed' do + before do + post(:revert, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: commit.id) + end + + it 'should redirect to the commit page' do + # Reverting a commit that has been already reverted. + post(:revert, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: commit.id) + + expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id) + expect(flash[:alert]).to match('Sorry, we cannot revert this commit automatically.') + end + end + end + + describe '#cherry_pick' do + context 'when target branch is not provided' do + it 'should render the 404 page' do + post(:cherry_pick, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: master_pickable_commit.id) + + expect(response).not_to be_success + expect(response).to have_http_status(404) + end + end + + context 'when the cherry-pick was successful' do + it 'should redirect to the commits page' do + post(:cherry_pick, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: master_pickable_commit.id) + + expect(response).to redirect_to namespace_project_commits_path(project.namespace, project, 'master') + expect(flash[:notice]).to eq('The commit has been successfully cherry-picked.') + end end - def go(id:) - get :show, + context 'when the cherry_pick failed' do + before do + post(:cherry_pick, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: master_pickable_commit.id) + end + + it 'should redirect to the commit page' do + # Cherry-picking a commit that has been already cherry-picked. + post(:cherry_pick, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + target_branch: 'master', + id: master_pickable_commit.id) + + expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, master_pickable_commit.id) + expect(flash[:alert]).to match('Sorry, we cannot cherry-pick this commit automatically.') + end + end + end + + describe 'GET #diff_for_path' do + def diff_for_path(extra_params = {}) + params = { namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: id + project_id: project.to_param + } + + get :diff_for_path, params.merge(extra_params) + end + + let(:existing_path) { '.gitmodules' } + + context 'when the commit exists' do + context 'when the user has access to the project' do + context 'when the path exists in the diff' do + it 'enables diff notes' do + diff_for_path(id: commit.id, path: existing_path) + + expect(assigns(:diff_notes_disabled)).to be_falsey + expect(assigns(:comments_target)).to eq(noteable_type: 'Commit', + commit_id: commit.id) + end + + it 'only renders the diffs for the path given' do + expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| + expect(diffs.map(&:new_path)).to contain_exactly(existing_path) + meth.call(diffs, diff_refs, project) + end + + diff_for_path(id: commit.id, path: existing_path) + end + end + + context 'when the path does not exist in the diff' do + before { diff_for_path(id: commit.id, path: existing_path.succ) } + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end + + context 'when the user does not have access to the project' do + before do + project.team.truncate + diff_for_path(id: commit.id, path: existing_path) + end + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end + + context 'when the commit does not exist' do + before { diff_for_path(id: commit.id.succ, path: existing_path) } + + it 'returns a 404' do + expect(response).to have_http_status(404) + end end end end diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index 4018dac95a2..40a068a87bb 100644 --- a/spec/controllers/projects/compare_controller_spec.rb +++ b/spec/controllers/projects/compare_controller_spec.rb @@ -64,4 +64,73 @@ describe Projects::CompareController do expect(assigns(:commits)).to eq(nil) end end + + describe 'GET #diff_for_path' do + def diff_for_path(extra_params = {}) + params = { + namespace_id: project.namespace.to_param, + project_id: project.to_param + } + + get :diff_for_path, params.merge(extra_params) + end + + let(:existing_path) { 'files/ruby/feature.rb' } + + context 'when the from and to refs exist' do + context 'when the user has access to the project' do + context 'when the path exists in the diff' do + it 'disables diff notes' do + diff_for_path(from: ref_from, to: ref_to, path: existing_path) + + expect(assigns(:diff_notes_disabled)).to be_truthy + end + + it 'only renders the diffs for the path given' do + expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| + expect(diffs.map(&:new_path)).to contain_exactly(existing_path) + meth.call(diffs, diff_refs, project) + end + + diff_for_path(from: ref_from, to: ref_to, path: existing_path) + end + end + + context 'when the path does not exist in the diff' do + before { diff_for_path(from: ref_from, to: ref_to, path: existing_path.succ) } + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end + + context 'when the user does not have access to the project' do + before do + project.team.truncate + diff_for_path(from: ref_from, to: ref_to, path: existing_path) + end + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end + + context 'when the from ref does not exist' do + before { diff_for_path(from: ref_from.succ, to: ref_to, path: existing_path) } + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + + context 'when the to ref does not exist' do + before { diff_for_path(from: ref_from, to: ref_to.succ, path: existing_path) } + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index c4b57e77804..1b4c4dcd1e5 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -289,101 +289,215 @@ describe Projects::MergeRequestsController do end end - describe 'GET diffs' do - def go(format: 'html') - get :diffs, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: merge_request.iid, - format: format + describe 'GET #diffs' do + def go(extra_params = {}) + params = { + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: merge_request.iid + } + + get :diffs, params.merge(extra_params) end - context 'as html' do - it 'renders the diff template' do - go + context 'with default params' do + context 'as html' do + before { go(format: 'html') } - expect(response).to render_template('diffs') + it 'renders the diff template' do + expect(response).to render_template('diffs') + end end - end - context 'as json' do - it 'renders the diffs template to a string' do - go format: 'json' + context 'as json' do + before { go(format: 'json') } - expect(response).to render_template('projects/merge_requests/show/_diffs') - expect(JSON.parse(response.body)).to have_key('html') + it 'renders the diffs template to a string' do + expect(response).to render_template('projects/merge_requests/show/_diffs') + expect(JSON.parse(response.body)).to have_key('html') + end end - end - - context 'with forked projects with submodules' do - render_views - let(:project) { create(:project) } - let(:fork_project) { create(:forked_project_with_submodules) } - let(:merge_request) { create(:merge_request_with_diffs, source_project: fork_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) } + context 'with forked projects with submodules' do + render_views - before do - fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id) - fork_project.save - merge_request.reload - end + let(:project) { create(:project) } + let(:fork_project) { create(:forked_project_with_submodules) } + let(:merge_request) { create(:merge_request_with_diffs, source_project: fork_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) } - it 'renders' do - go format: 'json' + before do + fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id) + fork_project.save + merge_request.reload + go(format: 'json') + end - expect(response).to be_success - expect(response.body).to have_content('Subproject commit') + it 'renders' do + expect(response).to be_success + expect(response.body).to have_content('Subproject commit') + end end end - end - describe 'GET diffs with ignore_whitespace_change' do - def go(format: 'html') - get :diffs, - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: merge_request.iid, - format: format, - w: 1 - end + context 'with ignore_whitespace_change' do + context 'as html' do + before { go(format: 'html', w: 1) } - context 'as html' do - it 'renders the diff template' do - go + it 'renders the diff template' do + expect(response).to render_template('diffs') + end + end + + context 'as json' do + before { go(format: 'json', w: 1) } - expect(response).to render_template('diffs') + it 'renders the diffs template to a string' do + expect(response).to render_template('projects/merge_requests/show/_diffs') + expect(JSON.parse(response.body)).to have_key('html') + end end end - context 'as json' do - it 'renders the diffs template to a string' do - go format: 'json' + context 'with view' do + before { go(view: 'parallel') } - expect(response).to render_template('projects/merge_requests/show/_diffs') - expect(JSON.parse(response.body)).to have_key('html') + it 'saves the preferred diff view in a cookie' do + expect(response.cookies['diff_view']).to eq('parallel') end end end - describe 'GET diffs with view' do - def go(extra_params = {}) + describe 'GET #diff_for_path' do + def diff_for_path(extra_params = {}) params = { namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: merge_request.iid + project_id: project.to_param } - get :diffs, params.merge(extra_params) + get :diff_for_path, params.merge(extra_params) end - it 'saves the preferred diff view in a cookie' do - go view: 'parallel' + context 'when an ID param is passed' do + let(:existing_path) { 'files/ruby/popen.rb' } - expect(response.cookies['diff_view']).to eq('parallel') + context 'when the merge request exists' do + context 'when the user can view the merge request' do + context 'when the path exists in the diff' do + it 'enables diff notes' do + diff_for_path(id: merge_request.iid, path: existing_path) + + expect(assigns(:diff_notes_disabled)).to be_falsey + expect(assigns(:comments_target)).to eq(noteable_type: 'MergeRequest', + noteable_id: merge_request.id) + end + + it 'only renders the diffs for the path given' do + expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| + expect(diffs.map(&:new_path)).to contain_exactly(existing_path) + meth.call(diffs, diff_refs, project) + end + + diff_for_path(id: merge_request.iid, path: existing_path) + end + end + + context 'when the path does not exist in the diff' do + before { diff_for_path(id: merge_request.iid, path: 'files/ruby/nopen.rb') } + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end + + context 'when the user cannot view the merge request' do + before do + project.team.truncate + diff_for_path(id: merge_request.iid, path: existing_path) + end + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end + + context 'when the merge request does not exist' do + before { diff_for_path(id: merge_request.iid.succ, path: existing_path) } + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + + context 'when the merge request belongs to a different project' do + let(:other_project) { create(:empty_project) } + + before do + other_project.team << [user, :master] + diff_for_path(id: merge_request.iid, path: existing_path, project_id: other_project.to_param) + end + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end + + context 'when source and target params are passed' do + let(:existing_path) { 'files/ruby/feature.rb' } + + context 'when both branches are in the same project' do + it 'disables diff notes' do + diff_for_path(path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' }) + + expect(assigns(:diff_notes_disabled)).to be_truthy + end + + it 'only renders the diffs for the path given' do + expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| + expect(diffs.map(&:new_path)).to contain_exactly(existing_path) + meth.call(diffs, diff_refs, project) + end + + diff_for_path(path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' }) + end + end + + context 'when the source branch is in a different project to the target' do + let(:other_project) { create(:project) } + + before { other_project.team << [user, :master] } + + context 'when the path exists in the diff' do + it 'disables diff notes' do + diff_for_path(path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) + + expect(assigns(:diff_notes_disabled)).to be_truthy + end + + it 'only renders the diffs for the path given' do + expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project| + expect(diffs.map(&:new_path)).to contain_exactly(existing_path) + meth.call(diffs, diff_refs, project) + end + + diff_for_path(path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) + end + end + + context 'when the path does not exist in the diff' do + before { diff_for_path(path: 'files/ruby/nopen.rb', merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) } + + it 'returns a 404' do + expect(response).to have_http_status(404) + end + end + end end end - describe 'GET commits' do + describe 'GET #commits' do def go(format: 'html') get :commits, namespace_id: project.namespace.to_param, diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb new file mode 100644 index 00000000000..3f5763b6408 --- /dev/null +++ b/spec/models/merge_request_diff_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +describe MergeRequestDiff, models: true do + describe '#diffs' do + let(:mr) { create(:merge_request, :with_diffs) } + let(:mr_diff) { mr.merge_request_diff } + + context 'when the :ignore_whitespace_change option is set' do + it 'creates a new compare object instead of loading from the DB' do + expect(mr_diff).not_to receive(:load_diffs) + expect(Gitlab::Git::Compare).to receive(:new).and_call_original + + mr_diff.diffs(ignore_whitespace_change: true) + end + end + + context 'when the raw diffs are empty' do + before { mr_diff.update_attributes(st_diffs: '') } + + it 'returns an empty DiffCollection' do + expect(mr_diff.diffs).to be_a(Gitlab::Git::DiffCollection) + expect(mr_diff.diffs).to be_empty + end + end + + context 'when the raw diffs exist' do + it 'returns the diffs' do + expect(mr_diff.diffs).to be_a(Gitlab::Git::DiffCollection) + expect(mr_diff.diffs).not_to be_empty + end + + context 'when the :paths option is set' do + let(:diffs) { mr_diff.diffs(paths: ['.gitignore', 'files/ruby/popen.rb', 'files/ruby/string.rb']) } + + it 'only returns diffs that match the paths given' do + expect(diffs.map(&:new_path)).to contain_exactly('.gitignore', 'files/ruby/popen.rb') + end + + it 'uses the diffs from the DB' do + expect(mr_diff).to receive(:load_diffs) + + diffs + end + end + end + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index a4b6ff8f8ad..c8ad7ab3e7f 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -116,6 +116,31 @@ describe MergeRequest, models: true do end end + describe '#diffs' do + let(:merge_request) { build(:merge_request) } + let(:options) { { paths: ['a/b', 'b/a', 'c/*'] } } + + context 'when there are MR diffs' do + it 'delegates to the MR diffs' do + merge_request.merge_request_diff = MergeRequestDiff.new + + expect(merge_request.merge_request_diff).to receive(:diffs).with(options) + + merge_request.diffs(options) + end + end + + context 'when there are no MR diffs' do + it 'delegates to the compare object' do + merge_request.compare = double(:compare) + + expect(merge_request.compare).to receive(:diffs).with(options) + + merge_request.diffs(options) + end + end + end + describe "#mr_and_commit_notes" do let!(:merge_request) { create(:merge_request) } -- cgit v1.2.1 From 78496e8c38ce0b415fa7aad5310b937a95265627 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 30 Jun 2016 16:17:10 +0100 Subject: Disable overflow messages With the option to expand and collapse individual diffs, these aren't needed any more. --- app/helpers/diff_helper.rb | 10 +--------- app/views/projects/diffs/_text_file.html.haml | 7 +------ app/views/projects/diffs/_warning.html.haml | 3 --- features/project/commits/commits.feature | 5 ----- features/steps/project/commits/commits.rb | 19 ------------------- spec/helpers/diff_helper_spec.rb | 17 +---------------- 6 files changed, 3 insertions(+), 58 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index d4655d60799..9a5920edfa2 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -37,16 +37,8 @@ module DiffHelper end end - def diff_hard_limit_enabled? - params[:force_show_diff].present? - end - def diff_options - options = { ignore_whitespace_change: hide_whitespace? } - if diff_hard_limit_enabled? - options.merge!(Commit.max_diff_options) - end - options + Commit.max_diff_options.merge(ignore_whitespace_change: hide_whitespace?) end def safe_diff_files(diffs, diff_refs: nil, repository: nil) diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 192093d1273..d61292c4bcb 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -1,9 +1,4 @@ -- too_big = diff_file.diff_lines.count > Commit::DIFF_SAFE_LINES -- if too_big - .suppressed-container - %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show. - -%table.text-file.code.js-syntax-highlight{ class: too_big ? 'hide' : '' } +%table.text-file.code.js-syntax-highlight - last_line = 0 - diff_file.highlighted_diff_lines.each do |line| - last_line = line.new_pos diff --git a/app/views/projects/diffs/_warning.html.haml b/app/views/projects/diffs/_warning.html.haml index 15536c17f8e..10fa1ddf2e5 100644 --- a/app/views/projects/diffs/_warning.html.haml +++ b/app/views/projects/diffs/_warning.html.haml @@ -2,9 +2,6 @@ %h4 Too many changes to show. .pull-right - - unless diff_hard_limit_enabled? - = link_to "Reload with full diff", url_for(params.merge(force_show_diff: true, format: nil)), class: "btn btn-sm" - - if current_controller?(:commit) or current_controller?(:merge_requests) - if current_controller?(:commit) = link_to "Plain diff", namespace_project_commit_path(@project.namespace, @project, @commit, format: :diff), class: "btn btn-sm" diff --git a/features/project/commits/commits.feature b/features/project/commits/commits.feature index a95df038357..8b0cb90765e 100644 --- a/features/project/commits/commits.feature +++ b/features/project/commits/commits.feature @@ -83,11 +83,6 @@ Feature: Project Commits #Given I visit my project's commits stats page #Then I see commits stats - Scenario: I browse big commit - Given I visit big commit page - Then I see big commit warning - And I see "Reload with full diff" link - Scenario: I browse a commit with an image Given I visit a commit with an image that changed Then The diff links to both the previous and current image diff --git a/features/steps/project/commits/commits.rb b/features/steps/project/commits/commits.rb index 239036e431d..bea9f9d198b 100644 --- a/features/steps/project/commits/commits.rb +++ b/features/steps/project/commits/commits.rb @@ -125,25 +125,6 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps expect(page).to have_content 'Authors' end - step 'I visit big commit page' do - # Create a temporary scope to ensure that the stub_const is removed after user - RSpec::Mocks.with_temporary_scope do - stub_const('Gitlab::Git::DiffCollection::DEFAULT_LIMITS', { max_lines: 1, max_files: 1 }) - visit namespace_project_commit_path(@project.namespace, @project, sample_big_commit.id) - end - end - - step 'I see big commit warning' do - expect(page).to have_content sample_big_commit.message - expect(page).to have_content "Too many changes" - end - - step 'I see "Reload with full diff" link' do - link = find_link('Reload with full diff') - expect(link[:href]).to end_with('?force_show_diff=true') - expect(link[:href]).not_to include('.html') - end - step 'I visit a commit with an image that changed' do visit namespace_project_commit_path(@project.namespace, @project, sample_image_commit.id) end diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index e2db33d8345..65ca8760958 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -31,26 +31,11 @@ describe DiffHelper do end end - describe 'diff_hard_limit_enabled?' do - it 'should return true if param is provided' do - allow(controller).to receive(:params) { { force_show_diff: true } } - expect(diff_hard_limit_enabled?).to be_truthy - end - - it 'should return false if param is not provided' do - expect(diff_hard_limit_enabled?).to be_falsey - end - end - describe 'diff_options' do - it 'should return hard limit for a diff if force diff is true' do + it 'should return hard limit for a diff' do allow(controller).to receive(:params) { { force_show_diff: true } } expect(diff_options).to include(Commit.max_diff_options) end - - it 'should return safe limit for a diff if force diff is false' do - expect(diff_options).not_to include(:max_lines, :max_files) - end end describe 'unfold_bottom_class' do -- cgit v1.2.1 From b8d3016abbfeaa0658216a9d21138435f2379e38 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Wed, 29 Jun 2016 17:14:17 +0100 Subject: Added frontend collapsible behavior --- app/assets/javascripts/diff.js.coffee | 2 + .../javascripts/merge_request_tabs.js.coffee | 1 + app/assets/javascripts/single_diff.js.coffee | 52 ++++++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 app/assets/javascripts/single_diff.js.coffee diff --git a/app/assets/javascripts/diff.js.coffee b/app/assets/javascripts/diff.js.coffee index 6d9b364cb8d..49c43c09983 100644 --- a/app/assets/javascripts/diff.js.coffee +++ b/app/assets/javascripts/diff.js.coffee @@ -1,6 +1,8 @@ class @Diff UNFOLD_COUNT = 20 constructor: -> + $('.files .diff-file').singleDiff() + $(document).off('click', '.js-unfold') $(document).on('click', '.js-unfold', (event) => target = $(event.target) diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee index 894f80586f1..728ee5a2aa2 100644 --- a/app/assets/javascripts/merge_request_tabs.js.coffee +++ b/app/assets/javascripts/merge_request_tabs.js.coffee @@ -160,6 +160,7 @@ class @MergeRequestTabs $('#diffs').html data.html gl.utils.localTimeAgo($('.js-timeago', 'div#diffs')) $('#diffs .js-syntax-highlight').syntaxHighlight() + $('#diffs .diff-file').singleDiff() @expandViewContainer() if @diffViewType() is 'parallel' @diffsLoaded = true @scrollToElement("#diffs") diff --git a/app/assets/javascripts/single_diff.js.coffee b/app/assets/javascripts/single_diff.js.coffee new file mode 100644 index 00000000000..4d1c28c082b --- /dev/null +++ b/app/assets/javascripts/single_diff.js.coffee @@ -0,0 +1,52 @@ +class @SingleDiff + + LOADING_HTML = '' + ERROR_HTML = '
Could not load diff
' + + constructor: (@file) -> + @content = $('.diff-content', @file) + @diffForPath = @content.data 'diff-for-path' + @setOpenState() + + $('.file-title > a', @file).on 'click', @toggleDiff + + setOpenState: -> + if @diffForPath + @isOpen = false + else + @isOpen = true + @contentHTML = @content.html() + return + + toggleDiff: (e) => + e.preventDefault() + @isOpen = !@isOpen + if not @isOpen and not @hasError + @content.empty() + return + if @contentHTML + @setContentHTML() + else + @getContentHTML() + return + + getContentHTML: -> + @content.html(LOADING_HTML).addClass 'loading' + $.get @diffForPath, (data) => + if data.html + @setContentHTML data.html + else + @hasError = true + @content.html ERROR_HTML + @content.removeClass 'loading' + return + + setContentHTML: (contentHTML) -> + @contentHTML = contentHTML if contentHTML + @content.html @contentHTML + @content.syntaxHighlight() + +$.fn.singleDiff = -> + return @each -> + if not $.data this, 'singleDiff' + $.data this, 'singleDiff', new SingleDiff this -- cgit v1.2.1 From 90a6be190feea0966e9ed9b6731d930bcff32d68 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 6 Jul 2016 12:36:39 +0100 Subject: 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 +- .../merge_requests/expand_collapse_diffs_spec.rb | 138 +++++++++++++++++++++ .../projects/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 = '' ERROR_HTML = '
Could not load diff
' + COLLAPSED_HTML = '
This diff is collapsed. Click to expand it.
' 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| -- cgit v1.2.1 From c082d92fb959ee2344b90b7fd4e316019452c094 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 6 Jul 2016 13:04:52 +0100 Subject: Allow expanding all diffs at once --- app/helpers/diff_helper.rb | 4 +++ app/views/projects/diffs/_content.html.haml | 2 +- app/views/projects/diffs/_diffs.html.haml | 2 ++ .../merge_requests/expand_collapse_diffs_spec.rb | 38 ++++++++++++++++++++++ 4 files changed, 45 insertions(+), 1 deletion(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 93f12198a58..fd7b71407f3 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -8,6 +8,10 @@ module DiffHelper [marked_old_line, marked_new_line] end + def expand_all? + @expand_all || params[:expand].present? + end + def render_diff_for_path(diffs, diff_refs, project) diff_file = safe_diff_files(diffs, diff_refs).first diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index 9c6a17c0a8c..5f4572ab9b0 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -9,7 +9,7 @@ - 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_file.collapsed_by_default? && !@expand_all + - 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. diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 1975287faee..7b530af0f4a 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -6,6 +6,8 @@ .content-block.oneline-block.files-changed .inline-parallel-buttons + - unless expand_all? + = link_to 'Expand all', url_for(params.merge(expand: 1, format: 'html')), class: 'btn btn-default' - if show_whitespace_toggle - if current_controller?(:commit) = commit_diff_whitespace_link(@project, @commit, class: 'hidden-xs') diff --git a/spec/features/merge_requests/expand_collapse_diffs_spec.rb b/spec/features/merge_requests/expand_collapse_diffs_spec.rb index 173ea3720da..f9c89c4ad96 100644 --- a/spec/features/merge_requests/expand_collapse_diffs_spec.rb +++ b/spec/features/merge_requests/expand_collapse_diffs_spec.rb @@ -135,4 +135,42 @@ feature 'Expand and collapse diffs', js: true, feature: true do end end end + + context 'expanding all diffs' do + before do + click_link('Expand all') + wait_for_ajax + execute_script('window.ajaxUris = []; $(document).ajaxSend(function(event, xhr, settings) { ajaxUris.push(settings.url) });') + end + + it 'reloads the page with all diffs expanded' do + expect(small_diff).to have_selector('.code') + expect(small_diff).not_to have_selector('.nothing-here-block') + + expect(large_diff).to have_selector('.code') + expect(large_diff).not_to have_selector('.nothing-here-block') + 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 -- cgit v1.2.1 From 4add7f65bc925fac3f6380f896fb2eccd236b2f7 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 6 Jul 2016 13:54:38 +0100 Subject: Fix comments on collapsed and expanded diffs We can't save the HTML as it was on page load, because comments etc. add content that we would lose if we kept the initial HTML. Instead, shuffle elements around. --- app/assets/javascripts/single_diff.js.coffee | 49 +++++++++++++--------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/app/assets/javascripts/single_diff.js.coffee b/app/assets/javascripts/single_diff.js.coffee index 76db716317b..884d5d43d03 100644 --- a/app/assets/javascripts/single_diff.js.coffee +++ b/app/assets/javascripts/single_diff.js.coffee @@ -1,5 +1,6 @@ class @SingleDiff + WRAPPER = '
' LOADING_HTML = '' ERROR_HTML = '
Could not load diff
' COLLAPSED_HTML = '
This diff is collapsed. Click to expand it.
' @@ -7,51 +8,47 @@ class @SingleDiff constructor: (@file) -> @content = $('.diff-content', @file) @diffForPath = @content.find('[data-diff-for-path]').data 'diff-for-path' - @setOpenState() + @isOpen = !@diffForPath - $('.file-title > a', @file).on 'click', @toggleDiff - @enableToggleOnContent() - - setOpenState: -> if @diffForPath - @isOpen = false + @collapsedContent = @content + @loadingContent = $(WRAPPER).addClass('loading').html(LOADING_HTML).hide() + @content = null + @collapsedContent.after(@loadingContent) else - @isOpen = true - @contentHTML = @content.html() - return + @collapsedContent = $(WRAPPER).html(COLLAPSED_HTML).hide() + @content.after(@collapsedContent) - enableToggleOnContent: -> - @content.find('.nothing-here-block.diff-collapsed').on 'click', @toggleDiff + @collapsedContent.on 'click', @toggleDiff + + $('.file-title > a', @file).on 'click', @toggleDiff toggleDiff: (e) => e.preventDefault() @isOpen = !@isOpen if not @isOpen and not @hasError - @content.html COLLAPSED_HTML - @enableToggleOnContent - return - if @contentHTML - @setContentHTML() + @content.hide() + @collapsedContent.show() + else if @content + @collapsedContent.hide() + @content.show() else @getContentHTML() - return getContentHTML: -> - @content.html(LOADING_HTML).addClass 'loading' + @collapsedContent.hide() + @loadingContent.show() $.get @diffForPath, (data) => + @loadingContent.hide() if data.html - @setContentHTML data.html + @content = $(data.html) + @content.syntaxHighlight() else @hasError = true - @content.html ERROR_HTML - @content.removeClass 'loading' + @content = $(ERROR_HTML) + @collapsedContent.after(@content) return - setContentHTML: (contentHTML) -> - @contentHTML = contentHTML if contentHTML - @content.html @contentHTML - @content.syntaxHighlight() - $.fn.singleDiff = -> return @each -> if not $.data this, 'singleDiff' -- cgit v1.2.1 From 6a46926f88d504778ae49f7824d2b1284a1c62ff Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 6 Jul 2016 17:51:02 +0100 Subject: Remove unused argument to CompareService#execute --- app/controllers/projects/compare_controller.rb | 14 ++++++++++---- app/services/compare_service.rb | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 8a04f63f4d4..5e00d2d5aff 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -14,7 +14,7 @@ class Projects::CompareController < Projects::ApplicationController def show compare = CompareService.new. - execute(@project, @head_ref, @project, @start_ref, diff_options) + execute(@project, @head_ref, @project, @start_ref) if compare @commits = Commit.decorate(compare.commits, @project) @@ -37,18 +37,24 @@ class Projects::CompareController < Projects::ApplicationController def diff_for_path compare = CompareService.new. - execute(@project, @head_ref, @project, @base_ref, diff_options) + execute(@project, @head_ref, @project, @start_ref) return render_404 unless compare + @start_commit = @project.commit(@start_ref) @commit = @project.commit(@head_ref) - @base_commit = @project.merge_base_commit(@base_ref, @head_ref) + @base_commit = @project.merge_base_commit(@start_ref, @head_ref) diffs = compare.diffs(diff_options.merge(paths: [params[:path]])) + diff_refs = Gitlab::Diff::DiffRefs.new( + base_sha: @base_commit.try(:sha), + start_sha: @start_commit.try(:sha), + head_sha: @commit.try(:sha) + ) @diff_notes_disabled = true @grouped_diff_notes = {} - render_diff_for_path(diffs, [@base_commit, @commit], @project) + render_diff_for_path(diffs, diff_refs, @project) end def create diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index e2bccbdbcc3..149822aa647 100644 --- a/app/services/compare_service.rb +++ b/app/services/compare_service.rb @@ -3,7 +3,7 @@ require 'securerandom' # Compare 2 branches for one repo or between repositories # and return Gitlab::Git::Compare object that responds to commits and diffs class CompareService - def execute(source_project, source_branch, target_project, target_branch, diff_options = {}) + def execute(source_project, source_branch, target_project, target_branch) source_commit = source_project.commit(source_branch) return unless source_commit -- cgit v1.2.1 From ff55398aafa2feccaba4ed470becabc526b4df48 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 6 Jul 2016 18:15:27 +0100 Subject: DRY up diff_for_path actions 1. Move render method to a concern, not a helper. 2. Let DiffHelper#diff_options automatically add the path option. 3. Move more instance var definitions to before filters. --- app/controllers/concerns/diff_for_path.rb | 23 ++++++++ app/controllers/projects/commit_controller.rb | 61 +++++++++---------- app/controllers/projects/compare_controller.rb | 68 +++++++++------------- .../projects/merge_requests_controller.rb | 6 +- app/helpers/diff_helper.rb | 24 +------- 5 files changed, 84 insertions(+), 98 deletions(-) create mode 100644 app/controllers/concerns/diff_for_path.rb diff --git a/app/controllers/concerns/diff_for_path.rb b/app/controllers/concerns/diff_for_path.rb new file mode 100644 index 00000000000..b9b5d136bd9 --- /dev/null +++ b/app/controllers/concerns/diff_for_path.rb @@ -0,0 +1,23 @@ +module DiffForPath + extend ActiveSupport::Concern + + def render_diff_for_path(diffs, diff_refs, project) + diff_file = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository).first + + return render_404 unless diff_file + + diff_commit = commit_for_diff(diff_file) + blob = diff_file.blob(diff_commit) + @expand_all = true + + locals = { + diff_file: diff_file, + diff_commit: diff_commit, + diff_refs: diff_refs, + blob: blob, + project: project + } + + render json: { html: view_to_html_string('projects/diffs/_content', locals) } + end +end diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 810653b4264..727e84b40a1 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -3,6 +3,7 @@ # Not to be confused with CommitsController, plural. class Projects::CommitController < Projects::ApplicationController include CreatesCommit + include DiffForPath include DiffHelper # Authorize @@ -11,29 +12,14 @@ class Projects::CommitController < Projects::ApplicationController before_action :authorize_update_build!, only: [:cancel_builds, :retry_builds] before_action :authorize_read_commit_status!, only: [:builds] before_action :commit - before_action :define_show_vars, only: [:show, :builds] + before_action :define_commit_vars, only: [:show, :diff_for_path, :builds] + before_action :define_status_vars, only: [:show, :builds] + before_action :define_note_vars, only: [:show, :diff_for_path] before_action :authorize_edit_tree!, only: [:revert, :cherry_pick] def show apply_diff_view_cookie! - @grouped_diff_notes = commit.notes.grouped_diff_notes - @notes = commit.notes.non_diff_notes.fresh - - Banzai::NoteRenderer.render( - @grouped_diff_notes.values.flatten + @notes, - @project, - current_user, - ) - - @note = @project.build_commit_note(commit) - - @noteable = @commit - @comments_target = { - noteable_type: 'Commit', - commit_id: @commit.id - } - respond_to do |format| format.html format.diff { render text: @commit.to_diff } @@ -42,21 +28,7 @@ class Projects::CommitController < Projects::ApplicationController end def diff_for_path - return git_not_found! unless commit - - opts = diff_options - opts[:ignore_whitespace_change] = true if params[:format] == 'diff' - - diffs = commit.diffs(opts.merge(paths: [params[:path]])) - diff_refs = [commit.parent || commit, commit] - - @comments_target = { - noteable_type: 'Commit', - commit_id: @commit.id - } - @grouped_diff_notes = {} - - render_diff_for_path(diffs, diff_refs, @project) + render_diff_for_path(@diffs, @commit.diff_refs, @project) end def builds @@ -132,7 +104,7 @@ class Projects::CommitController < Projects::ApplicationController @ci_builds ||= Ci::Build.where(pipeline: pipelines) end - def define_show_vars + def define_commit_vars return git_not_found! unless commit opts = diff_options @@ -140,7 +112,28 @@ class Projects::CommitController < Projects::ApplicationController @diffs = commit.diffs(opts) @notes_count = commit.notes.count + end + + def define_note_vars + @grouped_diff_notes = commit.notes.grouped_diff_notes + @notes = commit.notes.non_diff_notes.fresh + + Banzai::NoteRenderer.render( + @grouped_diff_notes.values.flatten + @notes, + @project, + current_user, + ) + + @note = @project.build_commit_note(commit) + + @noteable = @commit + @comments_target = { + noteable_type: 'Commit', + commit_id: @commit.id + } + end + def define_status_vars @statuses = CommitStatus.where(pipeline: pipelines) @builds = Ci::Build.where(pipeline: pipelines) end diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 5e00d2d5aff..5f3ee71444d 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -1,60 +1,26 @@ require 'addressable/uri' class Projects::CompareController < Projects::ApplicationController + include DiffForPath include DiffHelper # Authorize before_action :require_non_empty_project before_action :authorize_download_code! - before_action :assign_ref_vars, only: [:index, :show, :diff_for_path] + before_action :define_ref_vars, only: [:index, :show, :diff_for_path] + before_action :define_diff_vars, only: [:show, :diff_for_path] before_action :merge_request, only: [:index, :show] def index end def show - compare = CompareService.new. - execute(@project, @head_ref, @project, @start_ref) - - if compare - @commits = Commit.decorate(compare.commits, @project) - - @start_commit = @project.commit(@start_ref) - @commit = @project.commit(@head_ref) - @base_commit = @project.merge_base_commit(@start_ref, @head_ref) - - @diffs = compare.diffs(diff_options) - @diff_refs = Gitlab::Diff::DiffRefs.new( - base_sha: @base_commit.try(:sha), - start_sha: @start_commit.try(:sha), - head_sha: @commit.try(:sha) - ) - - @diff_notes_disabled = true - @grouped_diff_notes = {} - end end def diff_for_path - compare = CompareService.new. - execute(@project, @head_ref, @project, @start_ref) - - return render_404 unless compare - - @start_commit = @project.commit(@start_ref) - @commit = @project.commit(@head_ref) - @base_commit = @project.merge_base_commit(@start_ref, @head_ref) - diffs = compare.diffs(diff_options.merge(paths: [params[:path]])) - diff_refs = Gitlab::Diff::DiffRefs.new( - base_sha: @base_commit.try(:sha), - start_sha: @start_commit.try(:sha), - head_sha: @commit.try(:sha) - ) - - @diff_notes_disabled = true - @grouped_diff_notes = {} + return render_404 unless @compare - render_diff_for_path(diffs, diff_refs, @project) + render_diff_for_path(@diffs, @diff_refs, @project) end def create @@ -64,11 +30,33 @@ class Projects::CompareController < Projects::ApplicationController private - def assign_ref_vars + def define_ref_vars @start_ref = Addressable::URI.unescape(params[:from]) @ref = @head_ref = Addressable::URI.unescape(params[:to]) end + def define_diff_vars + @compare = CompareService.new.execute(@project, @head_ref, @project, @start_ref) + + if @compare + @commits = Commit.decorate(@compare.commits, @project) + + @start_commit = @project.commit(@start_ref) + @commit = @project.commit(@head_ref) + @base_commit = @project.merge_base_commit(@start_ref, @head_ref) + + @diffs = @compare.diffs(diff_options) + @diff_refs = Gitlab::Diff::DiffRefs.new( + base_sha: @base_commit.try(:sha), + start_sha: @start_commit.try(:sha), + head_sha: @commit.try(:sha) + ) + + @diff_notes_disabled = true + @grouped_diff_notes = {} + end + end + def merge_request @merge_request ||= @project.merge_requests.opened. find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 3fc5a319c9b..31d7c324e55 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -1,5 +1,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController include ToggleSubscriptionAction + include DiffForPath include DiffHelper include IssuableActions include ToggleAwardEmoji @@ -102,10 +103,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController end define_commit_vars - diffs = @merge_request.diffs(diff_options.merge(paths: [params[:path]])) - diff_refs = @merge_request.diff_refs + diffs = @merge_request.diffs(diff_options) - render_diff_for_path(diffs, diff_refs, @merge_request.project) + render_diff_for_path(diffs, @merge_request.diff_refs, @merge_request.project) end def commits diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index fd7b71407f3..ebfe4e27b78 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -12,26 +12,6 @@ module DiffHelper @expand_all || params[:expand].present? end - def render_diff_for_path(diffs, diff_refs, project) - diff_file = safe_diff_files(diffs, diff_refs).first - - return render_404 unless diff_file - - 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_commit: diff_commit, - diff_refs: diff_refs, - blob: blob, - project: project - } - - render json: { html: view_to_html_string('projects/diffs/_content', locals) } - end - def diff_view diff_views = %w(inline parallel) @@ -43,7 +23,9 @@ module DiffHelper end def diff_options - Commit.max_diff_options.merge(ignore_whitespace_change: hide_whitespace?) + default_options = Commit.max_diff_options + default_options[:paths] = [params[:path]] if params[:path] + default_options.merge(ignore_whitespace_change: hide_whitespace?) end def safe_diff_files(diffs, diff_refs: nil, repository: nil) -- cgit v1.2.1 From ea1827c9a491de719be6a084fc092b88b1f54600 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 6 Jul 2016 18:35:55 +0100 Subject: Make expand_all param more explicit --- app/controllers/concerns/diff_for_path.rb | 2 +- app/helpers/diff_helper.rb | 4 ++-- app/views/projects/diffs/_content.html.haml | 2 +- app/views/projects/diffs/_diffs.html.haml | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/concerns/diff_for_path.rb b/app/controllers/concerns/diff_for_path.rb index b9b5d136bd9..5ca8d3af0c9 100644 --- a/app/controllers/concerns/diff_for_path.rb +++ b/app/controllers/concerns/diff_for_path.rb @@ -8,7 +8,7 @@ module DiffForPath diff_commit = commit_for_diff(diff_file) blob = diff_file.blob(diff_commit) - @expand_all = true + @expand_all_diffs = true locals = { diff_file: diff_file, diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index ebfe4e27b78..04490226e50 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -8,8 +8,8 @@ module DiffHelper [marked_old_line, marked_new_line] end - def expand_all? - @expand_all || params[:expand].present? + def expand_all_diffs? + @expand_all_diffs || params[:expand_all_diffs].present? end def diff_view diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index 5f4572ab9b0..bfcd3ee9132 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -9,7 +9,7 @@ - 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_file.collapsed_by_default? && !expand_all? + - if diff_file.collapsed_by_default? && !expand_all_diffs? - 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. diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 7b530af0f4a..5db70bbb478 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -6,8 +6,8 @@ .content-block.oneline-block.files-changed .inline-parallel-buttons - - unless expand_all? - = link_to 'Expand all', url_for(params.merge(expand: 1, format: 'html')), class: 'btn btn-default' + - unless expand_all_diffs? + = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: 'html')), class: 'btn btn-default' - if show_whitespace_toggle - if current_controller?(:commit) = commit_diff_whitespace_link(@project, @commit, class: 'hidden-xs') -- cgit v1.2.1 From 32e593c66323a2cc5cd3b25070c9ae8db725f5be Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Wed, 6 Jul 2016 21:11:44 +0100 Subject: review changes --- app/assets/javascripts/diff.js.coffee | 2 +- .../javascripts/merge_request_tabs.js.coffee | 2 +- app/assets/javascripts/single_diff.js.coffee | 55 ---------------------- app/assets/javascripts/single_file_diff.js.coffee | 54 +++++++++++++++++++++ app/assets/stylesheets/framework/blocks.scss | 3 ++ app/assets/stylesheets/framework/files.scss | 4 -- 6 files changed, 59 insertions(+), 61 deletions(-) delete mode 100644 app/assets/javascripts/single_diff.js.coffee create mode 100644 app/assets/javascripts/single_file_diff.js.coffee diff --git a/app/assets/javascripts/diff.js.coffee b/app/assets/javascripts/diff.js.coffee index 49c43c09983..feb908c1abb 100644 --- a/app/assets/javascripts/diff.js.coffee +++ b/app/assets/javascripts/diff.js.coffee @@ -1,7 +1,7 @@ class @Diff UNFOLD_COUNT = 20 constructor: -> - $('.files .diff-file').singleDiff() + $('.files .diff-file').singleFileDiff() $(document).off('click', '.js-unfold') $(document).on('click', '.js-unfold', (event) => diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee index 728ee5a2aa2..d55c4a34c07 100644 --- a/app/assets/javascripts/merge_request_tabs.js.coffee +++ b/app/assets/javascripts/merge_request_tabs.js.coffee @@ -160,7 +160,7 @@ class @MergeRequestTabs $('#diffs').html data.html gl.utils.localTimeAgo($('.js-timeago', 'div#diffs')) $('#diffs .js-syntax-highlight').syntaxHighlight() - $('#diffs .diff-file').singleDiff() + $('#diffs .diff-file').singleFileDiff() @expandViewContainer() if @diffViewType() is 'parallel' @diffsLoaded = true @scrollToElement("#diffs") diff --git a/app/assets/javascripts/single_diff.js.coffee b/app/assets/javascripts/single_diff.js.coffee deleted file mode 100644 index 884d5d43d03..00000000000 --- a/app/assets/javascripts/single_diff.js.coffee +++ /dev/null @@ -1,55 +0,0 @@ -class @SingleDiff - - WRAPPER = '
' - LOADING_HTML = '' - ERROR_HTML = '
Could not load diff
' - COLLAPSED_HTML = '
This diff is collapsed. Click to expand it.
' - - constructor: (@file) -> - @content = $('.diff-content', @file) - @diffForPath = @content.find('[data-diff-for-path]').data 'diff-for-path' - @isOpen = !@diffForPath - - if @diffForPath - @collapsedContent = @content - @loadingContent = $(WRAPPER).addClass('loading').html(LOADING_HTML).hide() - @content = null - @collapsedContent.after(@loadingContent) - else - @collapsedContent = $(WRAPPER).html(COLLAPSED_HTML).hide() - @content.after(@collapsedContent) - - @collapsedContent.on 'click', @toggleDiff - - $('.file-title > a', @file).on 'click', @toggleDiff - - toggleDiff: (e) => - e.preventDefault() - @isOpen = !@isOpen - if not @isOpen and not @hasError - @content.hide() - @collapsedContent.show() - else if @content - @collapsedContent.hide() - @content.show() - else - @getContentHTML() - - getContentHTML: -> - @collapsedContent.hide() - @loadingContent.show() - $.get @diffForPath, (data) => - @loadingContent.hide() - if data.html - @content = $(data.html) - @content.syntaxHighlight() - else - @hasError = true - @content = $(ERROR_HTML) - @collapsedContent.after(@content) - return - -$.fn.singleDiff = -> - return @each -> - if not $.data this, 'singleDiff' - $.data this, 'singleDiff', new SingleDiff this diff --git a/app/assets/javascripts/single_file_diff.js.coffee b/app/assets/javascripts/single_file_diff.js.coffee new file mode 100644 index 00000000000..f3e225c3728 --- /dev/null +++ b/app/assets/javascripts/single_file_diff.js.coffee @@ -0,0 +1,54 @@ +class @SingleFileDiff + + WRAPPER = '
' + LOADING_HTML = '' + ERROR_HTML = '
Could not load diff
' + COLLAPSED_HTML = '
This diff is collapsed. Click to expand it.
' + + constructor: (@file) -> + @content = $('.diff-content', @file) + @diffForPath = @content.find('[data-diff-for-path]').data 'diff-for-path' + @isOpen = !@diffForPath + + if @diffForPath + @collapsedContent = @content + @loadingContent = $(WRAPPER).addClass('loading').html(LOADING_HTML).hide() + @content = null + @collapsedContent.after(@loadingContent) + else + @collapsedContent = $(WRAPPER).html(COLLAPSED_HTML).hide() + @content.after(@collapsedContent) + + @collapsedContent.on 'click', @toggleDiff + + $('.file-title > a', @file).on 'click', @toggleDiff + + toggleDiff: (e) => + @isOpen = !@isOpen + if not @isOpen and not @hasError + @content.hide() + @collapsedContent.show() + else if @content + @collapsedContent.hide() + @content.show() + else + @getContentHTML() + + getContentHTML: -> + @collapsedContent.hide() + @loadingContent.show() + $.get @diffForPath, (data) => + @loadingContent.hide() + if data.html + @content = $(data.html) + @content.syntaxHighlight() + else + @hasError = true + @content = $(ERROR_HTML) + @collapsedContent.after(@content) + return + +$.fn.singleFileDiff = -> + return @each -> + if not $.data this, 'singleFileDiff' + $.data this, 'singleFileDiff', new SingleFileDiff this diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss index 41e77a4ac68..24b1ebab4b0 100644 --- a/app/assets/stylesheets/framework/blocks.scss +++ b/app/assets/stylesheets/framework/blocks.scss @@ -16,6 +16,9 @@ font-weight: normal; font-size: 16px; line-height: 36px; + &.diff-collapsed { + cursor: pointer; + } } .row-content-block { diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 02480689f09..71e4b50f2af 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -189,7 +189,3 @@ span.idiff { border-bottom-right-radius: 2px; } } - -.nothing-here-block.diff-collapsed { - cursor: pointer; -} -- cgit v1.2.1 From 87aba17ecdc74cac627dbd6731ea000933182f3a Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 8 Jul 2016 10:37:11 +0100 Subject: Memoize diffs with path options set separately --- app/models/merge_request_diff.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 06b28fc5a75..7e22491d0a3 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -46,7 +46,8 @@ class MergeRequestDiff < ActiveRecord::Base compare.diffs(options) end else - @diffs ||= load_diffs(st_diffs, options) + @diffs ||= {} + @diffs[options[:paths]] ||= load_diffs(st_diffs, options) end end -- cgit v1.2.1 From e462e122784f40550c53224af5a58b201ed1fd8f Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 8 Jul 2016 18:11:47 +0100 Subject: Tidy up spec action names --- spec/controllers/projects/commit_controller_spec.rb | 10 +++++----- spec/controllers/projects/compare_controller_spec.rb | 2 +- .../projects/merge_requests_controller_spec.rb | 16 ++++++++-------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index 9679b4f849f..472c4904919 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -12,7 +12,7 @@ describe Projects::CommitController do project.team << [user, :master] end - describe 'GET #show' do + describe 'GET show' do render_views def go(extra_params = {}) @@ -141,7 +141,7 @@ describe Projects::CommitController do end end - describe "#branches" do + describe "GET branches" do it "contains branch and tags information" do get(:branches, namespace_id: project.namespace.to_param, @@ -153,7 +153,7 @@ describe Projects::CommitController do end end - describe '#revert' do + describe 'POST revert' do context 'when target branch is not provided' do it 'should render the 404 page' do post(:revert, @@ -202,7 +202,7 @@ describe Projects::CommitController do end end - describe '#cherry_pick' do + describe 'POST cherry_pick' do context 'when target branch is not provided' do it 'should render the 404 page' do post(:cherry_pick, @@ -251,7 +251,7 @@ describe Projects::CommitController do end end - describe 'GET #diff_for_path' do + describe 'GET diff_for_path' do def diff_for_path(extra_params = {}) params = { namespace_id: project.namespace.to_param, diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index 40a068a87bb..1dd144a7b72 100644 --- a/spec/controllers/projects/compare_controller_spec.rb +++ b/spec/controllers/projects/compare_controller_spec.rb @@ -65,7 +65,7 @@ describe Projects::CompareController do end end - describe 'GET #diff_for_path' do + describe 'GET diff_for_path' do def diff_for_path(extra_params = {}) params = { namespace_id: project.namespace.to_param, diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 1b4c4dcd1e5..ff160f51329 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -10,7 +10,7 @@ describe Projects::MergeRequestsController do project.team << [user, :master] end - describe '#new' do + describe 'GET new' do context 'merge request that removes a submodule' do render_views @@ -34,7 +34,7 @@ describe Projects::MergeRequestsController do end end - describe "#show" do + describe "GET show" do shared_examples "export merge as" do |format| it "should generally work" do get(:show, @@ -108,7 +108,7 @@ describe Projects::MergeRequestsController do end end - describe 'GET #index' do + describe 'GET index' do def get_merge_requests get :index, namespace_id: project.namespace.to_param, @@ -140,7 +140,7 @@ describe Projects::MergeRequestsController do end end - describe 'PUT #update' do + describe 'PUT update' do context 'there is no source project' do let(:project) { create(:project) } let(:fork_project) { create(:forked_project_with_submodules) } @@ -168,7 +168,7 @@ describe Projects::MergeRequestsController do end end - describe 'POST #merge' do + describe 'POST merge' do let(:base_params) do { namespace_id: project.namespace.path, @@ -266,7 +266,7 @@ describe Projects::MergeRequestsController do end end - describe "DELETE #destroy" do + describe "DELETE destroy" do it "denies access to users unless they're admin or project owner" do delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid @@ -289,7 +289,7 @@ describe Projects::MergeRequestsController do end end - describe 'GET #diffs' do + describe 'GET diffs' do def go(extra_params = {}) params = { namespace_id: project.namespace.to_param, @@ -367,7 +367,7 @@ describe Projects::MergeRequestsController do end end - describe 'GET #diff_for_path' do + describe 'GET diff_for_path' do def diff_for_path(extra_params = {}) params = { namespace_id: project.namespace.to_param, -- cgit v1.2.1 From 5266ae87c43a6760600e397257f9791d950dbe15 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 8 Jul 2016 22:50:06 +0100 Subject: Support renames in diff_for_path actions --- app/helpers/diff_helper.rb | 8 +++-- app/models/merge_request_diff.rb | 5 +-- app/views/projects/diffs/_content.html.haml | 2 +- config/routes.rb | 8 ++--- .../controllers/projects/commit_controller_spec.rb | 10 +++--- .../projects/compare_controller_spec.rb | 12 +++---- .../projects/merge_requests_controller_spec.rb | 24 +++++++------- .../merge_requests/expand_collapse_diffs_spec.rb | 37 ++++++++++++++++++++-- spec/models/merge_request_diff_spec.rb | 6 ++-- spec/support/test_env.rb | 28 ++++++++-------- 10 files changed, 89 insertions(+), 51 deletions(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 04490226e50..e29f665baec 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -24,7 +24,11 @@ module DiffHelper def diff_options default_options = Commit.max_diff_options - default_options[:paths] = [params[:path]] if params[:path] + + if action_name == 'diff_for_path' + default_options[:paths] = params.values_at(:old_path, :new_path) + end + default_options.merge(ignore_whitespace_change: hide_whitespace?) end @@ -88,7 +92,7 @@ module DiffHelper def commit_for_diff(diff_file) return diff_file.content_commit if diff_file.content_commit - + if diff_file.deleted_file @base_commit || @commit.parent || @commit else diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 7e22491d0a3..d54369c3483 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -47,7 +47,7 @@ class MergeRequestDiff < ActiveRecord::Base end else @diffs ||= {} - @diffs[options[:paths]] ||= load_diffs(st_diffs, options) + @diffs[options] ||= load_diffs(st_diffs, options) end end @@ -146,8 +146,9 @@ class MergeRequestDiff < ActiveRecord::Base def load_diffs(raw, options) if raw.respond_to?(:each) if options[:paths] + old_path, new_path = options[:paths] raw = raw.select do |diff| - options[:paths].include?(diff[:new_path]) + old_path == diff[:old_path] && new_path == diff[:new_path] end end diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index bfcd3ee9132..0c0424edffd 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -10,7 +10,7 @@ .nothing-here-block This diff was suppressed by a .gitattributes entry. - elsif diff_file.diff_lines.length > 0 - if diff_file.collapsed_by_default? && !expand_all_diffs? - - url = url_for(params.merge(action: :diff_for_path, path: diff_file.file_path, format: nil)) + - url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path)) .nothing-here-block.diff-collapsed{data: { diff_for_path: url } } This diff is collapsed. Click to expand it. - elsif diff_view == 'parallel' diff --git a/config/routes.rb b/config/routes.rb index f31f8171993..b4f83c58bbd 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -615,13 +615,13 @@ Rails.application.routes.draw do post :retry_builds post :revert post :cherry_pick - get '/diffs/*path', action: :diff_for_path, constraints: { format: false } + get :diff_for_path end end resources :compare, only: [:index, :create] do collection do - get '/diffs/*path', action: :diff_for_path, constraints: { format: false } + get :diff_for_path end end @@ -711,14 +711,14 @@ Rails.application.routes.draw do post :toggle_subscription post :toggle_award_emoji post :remove_wip - get '/diffs/*path', action: :diff_for_path, constraints: { format: false } + get :diff_for_path end collection do get :branch_from get :branch_to get :update_branches - get '/diffs/*path', action: :diff_for_path, constraints: { format: false } + get :diff_for_path end end diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index 472c4904919..3001d32e719 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -267,7 +267,7 @@ describe Projects::CommitController do context 'when the user has access to the project' do context 'when the path exists in the diff' do it 'enables diff notes' do - diff_for_path(id: commit.id, path: existing_path) + diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path) expect(assigns(:diff_notes_disabled)).to be_falsey expect(assigns(:comments_target)).to eq(noteable_type: 'Commit', @@ -280,12 +280,12 @@ describe Projects::CommitController do meth.call(diffs, diff_refs, project) end - diff_for_path(id: commit.id, path: existing_path) + diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path) end end context 'when the path does not exist in the diff' do - before { diff_for_path(id: commit.id, path: existing_path.succ) } + before { diff_for_path(id: commit.id, old_path: existing_path.succ, new_path: existing_path.succ) } it 'returns a 404' do expect(response).to have_http_status(404) @@ -296,7 +296,7 @@ describe Projects::CommitController do context 'when the user does not have access to the project' do before do project.team.truncate - diff_for_path(id: commit.id, path: existing_path) + diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path) end it 'returns a 404' do @@ -306,7 +306,7 @@ describe Projects::CommitController do end context 'when the commit does not exist' do - before { diff_for_path(id: commit.id.succ, path: existing_path) } + before { diff_for_path(id: commit.id.succ, old_path: existing_path, new_path: existing_path) } it 'returns a 404' do expect(response).to have_http_status(404) diff --git a/spec/controllers/projects/compare_controller_spec.rb b/spec/controllers/projects/compare_controller_spec.rb index 1dd144a7b72..4058d5e2453 100644 --- a/spec/controllers/projects/compare_controller_spec.rb +++ b/spec/controllers/projects/compare_controller_spec.rb @@ -81,7 +81,7 @@ describe Projects::CompareController do context 'when the user has access to the project' do context 'when the path exists in the diff' do it 'disables diff notes' do - diff_for_path(from: ref_from, to: ref_to, path: existing_path) + diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path) expect(assigns(:diff_notes_disabled)).to be_truthy end @@ -92,12 +92,12 @@ describe Projects::CompareController do meth.call(diffs, diff_refs, project) end - diff_for_path(from: ref_from, to: ref_to, path: existing_path) + diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path) end end context 'when the path does not exist in the diff' do - before { diff_for_path(from: ref_from, to: ref_to, path: existing_path.succ) } + before { diff_for_path(from: ref_from, to: ref_to, old_path: existing_path.succ, new_path: existing_path.succ) } it 'returns a 404' do expect(response).to have_http_status(404) @@ -108,7 +108,7 @@ describe Projects::CompareController do context 'when the user does not have access to the project' do before do project.team.truncate - diff_for_path(from: ref_from, to: ref_to, path: existing_path) + diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path) end it 'returns a 404' do @@ -118,7 +118,7 @@ describe Projects::CompareController do end context 'when the from ref does not exist' do - before { diff_for_path(from: ref_from.succ, to: ref_to, path: existing_path) } + before { diff_for_path(from: ref_from.succ, to: ref_to, old_path: existing_path, new_path: existing_path) } it 'returns a 404' do expect(response).to have_http_status(404) @@ -126,7 +126,7 @@ describe Projects::CompareController do end context 'when the to ref does not exist' do - before { diff_for_path(from: ref_from, to: ref_to.succ, path: existing_path) } + before { diff_for_path(from: ref_from, to: ref_to.succ, old_path: existing_path, new_path: existing_path) } it 'returns a 404' do expect(response).to have_http_status(404) diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index ff160f51329..210085e3b1a 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -384,7 +384,7 @@ describe Projects::MergeRequestsController do context 'when the user can view the merge request' do context 'when the path exists in the diff' do it 'enables diff notes' do - diff_for_path(id: merge_request.iid, path: existing_path) + diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path) expect(assigns(:diff_notes_disabled)).to be_falsey expect(assigns(:comments_target)).to eq(noteable_type: 'MergeRequest', @@ -397,12 +397,12 @@ describe Projects::MergeRequestsController do meth.call(diffs, diff_refs, project) end - diff_for_path(id: merge_request.iid, path: existing_path) + diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path) end end context 'when the path does not exist in the diff' do - before { diff_for_path(id: merge_request.iid, path: 'files/ruby/nopen.rb') } + before { diff_for_path(id: merge_request.iid, old_path: 'files/ruby/nopen.rb', new_path: 'files/ruby/nopen.rb') } it 'returns a 404' do expect(response).to have_http_status(404) @@ -413,7 +413,7 @@ describe Projects::MergeRequestsController do context 'when the user cannot view the merge request' do before do project.team.truncate - diff_for_path(id: merge_request.iid, path: existing_path) + diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path) end it 'returns a 404' do @@ -423,7 +423,7 @@ describe Projects::MergeRequestsController do end context 'when the merge request does not exist' do - before { diff_for_path(id: merge_request.iid.succ, path: existing_path) } + before { diff_for_path(id: merge_request.iid.succ, old_path: existing_path, new_path: existing_path) } it 'returns a 404' do expect(response).to have_http_status(404) @@ -435,7 +435,7 @@ describe Projects::MergeRequestsController do before do other_project.team << [user, :master] - diff_for_path(id: merge_request.iid, path: existing_path, project_id: other_project.to_param) + diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path, project_id: other_project.to_param) end it 'returns a 404' do @@ -449,7 +449,7 @@ describe Projects::MergeRequestsController do context 'when both branches are in the same project' do it 'disables diff notes' do - diff_for_path(path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' }) + diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' }) expect(assigns(:diff_notes_disabled)).to be_truthy end @@ -460,7 +460,7 @@ describe Projects::MergeRequestsController do meth.call(diffs, diff_refs, project) end - diff_for_path(path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' }) + diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' }) end end @@ -471,7 +471,7 @@ describe Projects::MergeRequestsController do context 'when the path exists in the diff' do it 'disables diff notes' do - diff_for_path(path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) + diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) expect(assigns(:diff_notes_disabled)).to be_truthy end @@ -482,12 +482,12 @@ describe Projects::MergeRequestsController do meth.call(diffs, diff_refs, project) end - diff_for_path(path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) + diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) end end context 'when the path does not exist in the diff' do - before { diff_for_path(path: 'files/ruby/nopen.rb', merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) } + before { diff_for_path(old_path: 'files/ruby/nopen.rb', new_path: 'files/ruby/nopen.rb', merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' }) } it 'returns a 404' do expect(response).to have_http_status(404) @@ -497,7 +497,7 @@ describe Projects::MergeRequestsController do end end - describe 'GET #commits' do + describe 'GET commits' do def go(format: 'html') get :commits, namespace_id: project.namespace.to_param, diff --git a/spec/features/merge_requests/expand_collapse_diffs_spec.rb b/spec/features/merge_requests/expand_collapse_diffs_spec.rb index f9c89c4ad96..7a05bb47979 100644 --- a/spec/features/merge_requests/expand_collapse_diffs_spec.rb +++ b/spec/features/merge_requests/expand_collapse_diffs_spec.rb @@ -5,7 +5,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do before do login_as :admin - merge_request = create(:merge_request, source_branch: 'expand-collapse-diffs', target_branch: 'master') + merge_request = create(:merge_request, target_branch: 'expand-collapse-diffs-start', source_branch: 'expand-collapse-diffs') project = merge_request.source_project # Ensure that undiffable.md is in .gitattributes @@ -21,7 +21,12 @@ feature 'Expand and collapse diffs', js: true, feature: true do # 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| + files = [ + 'small_diff.md', 'large_diff.md', 'large_diff_renamed.md', 'undiffable.md', + 'too_large.md', 'too_large_image.jpg' + ] + + files.each do |file| define_method(file.split('.').first) { file_container(file) } end @@ -31,11 +36,18 @@ feature 'Expand and collapse diffs', js: true, feature: true do expect(small_diff).not_to have_selector('.nothing-here-block') end - it 'collapses larges diffs by default' do + it 'collapses large diffs by default' do expect(large_diff).not_to have_selector('.code') expect(large_diff).to have_selector('.nothing-here-block') end + it 'collapses large diffs for renamed files by default' do + expect(large_diff_renamed).not_to have_selector('.code') + expect(large_diff_renamed).to have_selector('.nothing-here-block') + expect(large_diff_renamed).to have_selector('.file-title .deletion') + expect(large_diff_renamed).to have_selector('.file-title .addition') + 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') @@ -53,6 +65,25 @@ feature 'Expand and collapse diffs', js: true, feature: true do expect(too_large_image).to have_selector('.image') end + context 'expanding a diff for a renamed file' do + before do + large_diff_renamed.find('.nothing-here-block').click + wait_for_ajax + end + + it 'shows the old content' do + old_line = large_diff_renamed.find('.line_content.old') + + expect(old_line).to have_content('four copies') + end + + it 'shows the new content' do + new_line = large_diff_renamed.find('.line_content.new', match: :prefer_exact) + + expect(new_line).to have_content('six copies') + end + end + context 'expanding a large diff' do before do click_link('large_diff.md') diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 3f5763b6408..9a637c94fbe 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -30,10 +30,10 @@ describe MergeRequestDiff, models: true do end context 'when the :paths option is set' do - let(:diffs) { mr_diff.diffs(paths: ['.gitignore', 'files/ruby/popen.rb', 'files/ruby/string.rb']) } + let(:diffs) { mr_diff.diffs(paths: ['files/ruby/popen.rb', 'files/ruby/popen.rb']) } - it 'only returns diffs that match the paths given' do - expect(diffs.map(&:new_path)).to contain_exactly('.gitignore', 'files/ruby/popen.rb') + it 'only returns diffs that match the (old path, new path) given' do + expect(diffs.map(&:new_path)).to contain_exactly('files/ruby/popen.rb') end it 'uses the diffs from the DB' do diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 6b99b0f24cb..311610c9911 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -5,19 +5,21 @@ module TestEnv # When developing the seed repository, comment out the branch you will modify. BRANCH_SHA = { - 'empty-branch' => '7efb185', - 'flatten-dir' => 'e56497b', - 'feature' => '0b4bc9a', - 'feature_conflict' => 'bb5206f', - 'fix' => '48f0be4', - 'improve/awesome' => '5937ac0', - 'markdown' => '0ed8c6c', - 'lfs' => 'be93687', - 'master' => '5937ac0', - "'test'" => 'e56497b', - 'orphaned-branch' => '45127a9', - 'binary-encoding' => '7b1cf43', - 'gitattributes' => '5a62481', + 'empty-branch' => '7efb185', + 'flatten-dir' => 'e56497b', + 'feature' => '0b4bc9a', + 'feature_conflict' => 'bb5206f', + 'fix' => '48f0be4', + 'improve/awesome' => '5937ac0', + 'markdown' => '0ed8c6c', + 'lfs' => 'be93687', + 'master' => '5937ac0', + "'test'" => 'e56497b', + 'orphaned-branch' => '45127a9', + 'binary-encoding' => '7b1cf43', + 'gitattributes' => '5a62481', + 'expand-collapse-diffs-start' => '65b04e4', + 'expand-collapse-diffs' => '865e6d5' } # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily -- cgit v1.2.1 From 93ab68607922e0bd0e22c40528a9d58fc50f50cd Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 11 Jul 2016 10:58:01 +0100 Subject: Fix feature specs on CI MySQL's text column isn't big enough for the diffs in the expand-collapse-diffs branch. --- spec/features/expand_collapse_diffs_spec.rb | 207 +++++++++++++++++++++ .../merge_requests/expand_collapse_diffs_spec.rb | 207 --------------------- spec/support/test_env.rb | 29 ++- 3 files changed, 221 insertions(+), 222 deletions(-) create mode 100644 spec/features/expand_collapse_diffs_spec.rb delete mode 100644 spec/features/merge_requests/expand_collapse_diffs_spec.rb diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb new file mode 100644 index 00000000000..7cff196c8d9 --- /dev/null +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -0,0 +1,207 @@ +require 'spec_helper' + +feature 'Expand and collapse diffs', js: true, feature: true do + include WaitForAjax + + before do + login_as :admin + project = create(:project) + branch = 'expand-collapse-diffs' + + # Ensure that undiffable.md is in .gitattributes + project.repository.copy_gitattributes(branch) + visit namespace_project_commit_path(project.namespace, project, project.commit(branch)) + 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. + # + files = [ + 'small_diff.md', 'large_diff.md', 'large_diff_renamed.md', 'undiffable.md', + 'too_large.md', 'too_large_image.jpg' + ] + + files.each do |file| + define_method(file.split('.').first) { file_container(file) } + end + + context 'visiting a commit with collapsed diffs' 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 large diffs by default' do + expect(large_diff).not_to have_selector('.code') + expect(large_diff).to have_selector('.nothing-here-block') + end + + it 'collapses large diffs for renamed files by default' do + expect(large_diff_renamed).not_to have_selector('.code') + expect(large_diff_renamed).to have_selector('.nothing-here-block') + expect(large_diff_renamed).to have_selector('.file-title .deletion') + expect(large_diff_renamed).to have_selector('.file-title .addition') + 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 diff for a renamed file' do + before do + large_diff_renamed.find('.nothing-here-block').click + wait_for_ajax + end + + it 'shows the old content' do + old_line = large_diff_renamed.find('.line_content.old') + + expect(old_line).to have_content('two copies') + end + + it 'shows the new content' do + new_line = large_diff_renamed.find('.line_content.new', match: :prefer_exact) + + expect(new_line).to have_content('three copies') + end + 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 + + context 'expanding all diffs' do + before do + click_link('Expand all') + wait_for_ajax + execute_script('window.ajaxUris = []; $(document).ajaxSend(function(event, xhr, settings) { ajaxUris.push(settings.url) });') + end + + it 'reloads the page with all diffs expanded' do + expect(small_diff).to have_selector('.code') + expect(small_diff).not_to have_selector('.nothing-here-block') + + expect(large_diff).to have_selector('.code') + expect(large_diff).not_to have_selector('.nothing-here-block') + 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/merge_requests/expand_collapse_diffs_spec.rb b/spec/features/merge_requests/expand_collapse_diffs_spec.rb deleted file mode 100644 index 7a05bb47979..00000000000 --- a/spec/features/merge_requests/expand_collapse_diffs_spec.rb +++ /dev/null @@ -1,207 +0,0 @@ -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, target_branch: 'expand-collapse-diffs-start', source_branch: 'expand-collapse-diffs') - 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. - # - files = [ - 'small_diff.md', 'large_diff.md', 'large_diff_renamed.md', 'undiffable.md', - 'too_large.md', 'too_large_image.jpg' - ] - - files.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 large diffs by default' do - expect(large_diff).not_to have_selector('.code') - expect(large_diff).to have_selector('.nothing-here-block') - end - - it 'collapses large diffs for renamed files by default' do - expect(large_diff_renamed).not_to have_selector('.code') - expect(large_diff_renamed).to have_selector('.nothing-here-block') - expect(large_diff_renamed).to have_selector('.file-title .deletion') - expect(large_diff_renamed).to have_selector('.file-title .addition') - 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 diff for a renamed file' do - before do - large_diff_renamed.find('.nothing-here-block').click - wait_for_ajax - end - - it 'shows the old content' do - old_line = large_diff_renamed.find('.line_content.old') - - expect(old_line).to have_content('four copies') - end - - it 'shows the new content' do - new_line = large_diff_renamed.find('.line_content.new', match: :prefer_exact) - - expect(new_line).to have_content('six copies') - end - 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 - - context 'expanding all diffs' do - before do - click_link('Expand all') - wait_for_ajax - execute_script('window.ajaxUris = []; $(document).ajaxSend(function(event, xhr, settings) { ajaxUris.push(settings.url) });') - end - - it 'reloads the page with all diffs expanded' do - expect(small_diff).to have_selector('.code') - expect(small_diff).not_to have_selector('.nothing-here-block') - - expect(large_diff).to have_selector('.code') - expect(large_diff).not_to have_selector('.nothing-here-block') - 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/support/test_env.rb b/spec/support/test_env.rb index 311610c9911..bb6c84262f6 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -5,21 +5,20 @@ module TestEnv # When developing the seed repository, comment out the branch you will modify. BRANCH_SHA = { - 'empty-branch' => '7efb185', - 'flatten-dir' => 'e56497b', - 'feature' => '0b4bc9a', - 'feature_conflict' => 'bb5206f', - 'fix' => '48f0be4', - 'improve/awesome' => '5937ac0', - 'markdown' => '0ed8c6c', - 'lfs' => 'be93687', - 'master' => '5937ac0', - "'test'" => 'e56497b', - 'orphaned-branch' => '45127a9', - 'binary-encoding' => '7b1cf43', - 'gitattributes' => '5a62481', - 'expand-collapse-diffs-start' => '65b04e4', - 'expand-collapse-diffs' => '865e6d5' + 'empty-branch' => '7efb185', + 'flatten-dir' => 'e56497b', + 'feature' => '0b4bc9a', + 'feature_conflict' => 'bb5206f', + 'fix' => '48f0be4', + 'improve/awesome' => '5937ac0', + 'markdown' => '0ed8c6c', + 'lfs' => 'be93687', + 'master' => '5937ac0', + "'test'" => 'e56497b', + 'orphaned-branch' => '45127a9', + 'binary-encoding' => '7b1cf43', + 'gitattributes' => '5a62481', + 'expand-collapse-diffs' => '4842455' } # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily -- cgit v1.2.1 From 8ee3c28bcde45e0719e0ef76731b9c92edfcddd9 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 11 Jul 2016 18:17:00 +0100 Subject: Handle more than two paths in MergeRequest#diffs --- app/controllers/concerns/diff_for_path.rb | 4 +++- app/models/merge_request_diff.rb | 5 ++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/controllers/concerns/diff_for_path.rb b/app/controllers/concerns/diff_for_path.rb index 5ca8d3af0c9..e09b8789eb2 100644 --- a/app/controllers/concerns/diff_for_path.rb +++ b/app/controllers/concerns/diff_for_path.rb @@ -2,7 +2,9 @@ module DiffForPath extend ActiveSupport::Concern def render_diff_for_path(diffs, diff_refs, project) - diff_file = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository).first + diff_file = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository).find do |diff| + diff.old_path == params[:old_path] && diff.new_path == params[:new_path] + end return render_404 unless diff_file diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index d54369c3483..feaba925bad 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -145,10 +145,9 @@ class MergeRequestDiff < ActiveRecord::Base def load_diffs(raw, options) if raw.respond_to?(:each) - if options[:paths] - old_path, new_path = options[:paths] + if paths = options[:paths] raw = raw.select do |diff| - old_path == diff[:old_path] && new_path == diff[:new_path] + paths.include?(diff[:old_path]) || paths.include?(diff[:new_path]) end end -- cgit v1.2.1