From ca3c0c6cd915d44ec2d409b04ab05d964bd5a403 Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Thu, 11 Aug 2016 09:00:17 +0200 Subject: [PATCH] MergeRequest new form load diff asynchronously --- CHANGELOG | 1 + .../javascripts/lib/utils/common_utils.js | 5 ++ app/assets/javascripts/merge_request_tabs.js | 23 +++++-- .../projects/merge_requests_controller.rb | 64 ++++++++++++------- app/models/merge_request.rb | 4 +- app/views/projects/diffs/_diffs.html.haml | 3 +- .../merge_requests/_new_diffs.html.haml | 1 + .../merge_requests/_new_submit.html.haml | 28 ++++---- config/routes/project.rb | 1 + features/project/source/browse_files.feature | 17 +++-- features/steps/project/source/browse_files.rb | 4 ++ 11 files changed, 100 insertions(+), 51 deletions(-) create mode 100644 app/views/projects/merge_requests/_new_diffs.html.haml diff --git a/CHANGELOG b/CHANGELOG index 68962f20d0b..91925847cab 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -30,6 +30,7 @@ v 8.13.0 (unreleased) - Replace `alias_method_chain` with `Module#prepend` - Enable GitLab Import/Export for non-admin users. - Preserve label filters when sorting !6136 (Joseph Frazier) + - MergeRequest#new form load diff asynchronously - Only update issuable labels if they have been changed - Take filters in account in issuable counters. !6496 - Use custom Ruby images to test builds (registry.dev.gitlab.org/gitlab/gitlab-build-images:*) diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 9299d0eabd2..b170e26eebf 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -38,6 +38,11 @@ gl.utils.getPagePath = function() { return $('body').data('page').split(':')[0]; }; + gl.utils.parseUrl = function (url) { + var parser = document.createElement('a'); + parser.href = url; + return parser; + }; return jQuery.timefor = function(time, suffix, expiredLabel) { var suffixFromNow, timefor; if (!time) { diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index bec11a198a1..8045d24a1bb 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -61,6 +61,9 @@ function MergeRequestTabs(opts) { this.opts = opts != null ? opts : {}; this.opts.setUrl = this.opts.setUrl !== undefined ? this.opts.setUrl : true; + + this.buildsLoaded = this.opts.buildsLoaded || false; + this.setCurrentAction = bind(this.setCurrentAction, this); this.tabShown = bind(this.tabShown, this); this.showTab = bind(this.showTab, this); @@ -93,7 +96,7 @@ this.loadCommits($target.attr('href')); this.expandView(); this.resetViewContainer(); - } else if (action === 'diffs') { + } else if (this.isDiffAction(action)) { this.loadDiff($target.attr('href')); if ((typeof bp !== "undefined" && bp !== null) && bp.getBreakpointSize() !== 'lg') { this.shrinkView(); @@ -170,8 +173,9 @@ action = 'notes'; } this.currentAction = action; - // Remove a trailing '/commits' or '/diffs' - new_state = this._location.pathname.replace(/\/(commits|diffs|builds|pipelines)(\.html)?\/?$/, ''); + // Remove a trailing '/commits' '/diffs' '/builds' '/pipelines' '/new' '/new/diffs' + new_state = this._location.pathname.replace(/\/(commits|diffs|builds|pipelines|new|new\/diffs)(\.html)?\/?$/, ''); + // Append the new action if we're on a tab other than 'notes' if (action !== 'notes') { new_state += "/" + action; @@ -210,8 +214,13 @@ if (this.diffsLoaded) { return; } + + // We extract pathname for the current Changes tab anchor href + // some pages like MergeRequestsController#new has query parameters on that anchor + var url = gl.utils.parseUrl(source); + return this._get({ - url: (source + ".json") + this._location.search, + url: (url.pathname + ".json") + this._location.search, success: (function(_this) { return function(data) { $('#diffs').html(data.html); @@ -223,7 +232,7 @@ gl.utils.localTimeAgo($('.js-timeago', 'div#diffs')); $('#diffs .js-syntax-highlight').syntaxHighlight(); $('#diffs .diff-file').singleFileDiff(); - if (_this.diffViewType() === 'parallel' && _this.currentAction === 'diffs') { + if (_this.diffViewType() === 'parallel' && (_this.isDiffAction(_this.currentAction)) ) { _this.expandViewContainer(); } _this.diffsLoaded = true; @@ -324,6 +333,10 @@ return $('.inline-parallel-buttons a.active').data('view-type'); }; + MergeRequestTabs.prototype.isDiffAction = function(action) { + return action === 'diffs' || action === 'new/diffs' + }; + MergeRequestTabs.prototype.expandViewContainer = function() { var $wrapper = $('.content-wrapper .container-fluid'); if (this.fixedLayoutPref === null) { diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 8c8c56228ad..ffd9833e3b1 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -19,6 +19,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController before_action :define_diff_comment_vars, only: [:diffs] before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds, :conflicts, :pipelines] before_action :close_merge_request_without_source_project, only: [:show, :diffs, :commits, :builds, :pipelines] + before_action :apply_diff_view_cookie!, only: [:new_diffs] + before_action :build_merge_request, only: [:new, :new_diffs] # Allow read any merge_request before_action :authorize_read_merge_request! @@ -210,29 +212,26 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def new - apply_diff_view_cookie! + define_new_vars + end - build_merge_request - @noteable = @merge_request + def new_diffs + respond_to do |format| + format.html do + define_new_vars + render "new" + end + format.json do + @diffs = if @merge_request.can_be_created + @merge_request.diffs(diff_options) + else + [] + end + @diff_notes_disabled = true - @target_branches = if @merge_request.target_project - @merge_request.target_project.repository.branch_names - else - [] - end - - @target_project = merge_request.target_project - @source_project = merge_request.source_project - @commits = @merge_request.compare_commits.reverse - @commit = @merge_request.diff_head_commit - @base_commit = @merge_request.diff_base_commit - @diffs = @merge_request.diffs(diff_options) if @merge_request.compare - @diff_notes_disabled = true - @pipeline = @merge_request.pipeline - @statuses = @pipeline.statuses.relevant if @pipeline - - @note_counts = Note.where(commit_id: @commits.map(&:id)). - group(:commit_id).count + render json: { html: view_to_html_string('projects/merge_requests/_new_diffs', diffs: @diffs) } + end + end end def create @@ -490,6 +489,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController ) end + def define_new_vars + @noteable = @merge_request + + @target_branches = if @merge_request.target_project + @merge_request.target_project.repository.branch_names + else + [] + end + + @target_project = merge_request.target_project + @source_project = merge_request.source_project + @commits = @merge_request.compare_commits.reverse + @commit = @merge_request.diff_head_commit + @base_commit = @merge_request.diff_base_commit + + @pipeline = @merge_request.pipeline + @statuses = @pipeline.statuses.relevant if @pipeline + @note_counts = Note.where(commit_id: @commits.map(&:id)). + group(:commit_id).count + end + def invalid_mr # Render special view for MR with removed target branch render 'invalid' @@ -521,7 +541,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController 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 + @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params.merge(diff_options: diff_options)).execute end def compared_diff_version diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 071dfe54ef9..a743bf313ae 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base # Temporary fields to store compare vars # when creating new merge request - attr_accessor :can_be_created, :compare_commits, :compare + attr_accessor :can_be_created, :compare_commits, :diff_options, :compare state_machine :state, initial: :opened do event :close do @@ -196,7 +196,7 @@ class MergeRequest < ActiveRecord::Base end def diff_size - merge_request_diff.size + diffs(diff_options).size end def diff_base_commit diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 576e7ef021a..067cf595da3 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,4 +1,5 @@ - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true) +- can_create_note = !@diff_notes_disabled && can?(current_user, :create_note, diffs.project) - diff_files = diffs.diff_files .content-block.oneline-block.files-changed @@ -20,7 +21,7 @@ - if diff_files.overflow? = render 'projects/diffs/warning', diff_files: diff_files -.files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, diffs.project))}} +.files{ data: { can_create_note: can_create_note } } - diff_files.each_with_index do |diff_file, index| - diff_commit = commit_for_diff(diff_file) - blob = diff_file.blob(diff_commit) diff --git a/app/views/projects/merge_requests/_new_diffs.html.haml b/app/views/projects/merge_requests/_new_diffs.html.haml new file mode 100644 index 00000000000..74367ab9b7b --- /dev/null +++ b/app/views/projects/merge_requests/_new_diffs.html.haml @@ -0,0 +1 @@ += render "projects/diffs/diffs", diffs: @diffs, show_whitespace_toggle: false diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index 00bd4e143df..88d8013a0d1 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -19,34 +19,32 @@ .mr-compare.merge-request %ul.merge-request-tabs.nav-links.no-top.no-bottom - %li.commits-tab + %li.commits-tab.active = link_to url_for(params), data: {target: 'div#commits', action: 'new', toggle: 'tab'} do Commits %span.badge= @commits.size - if @pipeline - %li.builds-tab.active + %li.builds-tab = link_to url_for(params), data: {target: 'div#builds', action: 'builds', toggle: 'tab'} do Builds %span.badge= @statuses.size - %li.diffs-tab.active - = link_to url_for(params), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do + %li.diffs-tab + = link_to url_for(params.merge(action: 'new_diffs')), data: {target: 'div#diffs', action: 'new/diffs', toggle: 'tab'} do Changes - %span.badge= @diffs.real_size + %span.badge= @merge_request.diff_size .tab-content - #commits.commits.tab-pane + #commits.commits.tab-pane.active = render "projects/merge_requests/show/commits" - #diffs.diffs.tab-pane.active - - if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE - .alert.alert-danger - %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits. - %p To preserve performance the line changes are not shown. - - else - = render "projects/diffs/diffs", diffs: @diffs, show_whitespace_toggle: false + #diffs.diffs.tab-pane + - # This tab is always loaded via AJAX - if @pipeline #builds.builds.tab-pane = render "projects/merge_requests/show/builds" + .mr-loading-status + = spinner + :javascript $('.assign-to-me-link').on('click', function(e){ $('#merge_request_assignee_id').val("#{current_user.id}").trigger("change"); @@ -54,6 +52,6 @@ }); :javascript var merge_request = new MergeRequest({ - action: "#{(@show_changes_tab ? 'diffs' : 'new')}", - setUrl: false + action: "#{(@show_changes_tab ? 'new/diffs' : 'new')}", + buildsLoaded: "#{@pipeline ? 'true' : 'false'}" }); diff --git a/config/routes/project.rb b/config/routes/project.rb index 224ec7e8324..0c188478ed1 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -285,6 +285,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only: get :update_branches get :diff_for_path post :bulk_update + get :new_diffs, path: 'new/diffs' end resources :discussions, only: [], constraints: { id: /\h{40}/ } do diff --git a/features/project/source/browse_files.feature b/features/project/source/browse_files.feature index fdffd71de85..d4b91fec6e8 100644 --- a/features/project/source/browse_files.feature +++ b/features/project/source/browse_files.feature @@ -71,6 +71,7 @@ Feature: Project Source Browse Files And I fill the new branch name And I click on "Commit Changes" Then I am redirected to the new merge request page + When I click on "Changes" tab And I should see its new content @javascript @@ -80,9 +81,10 @@ Feature: Project Source Browse Files And I fill the upload file commit message And I fill the new branch name And I click on "Upload file" - Then I can see the new text file + Then I can see the new commit message And I am redirected to the new merge request page - And I can see the new commit message + When I click on "Changes" tab + Then I can see the new text file @javascript Scenario: I can upload file and commit when I don't have write access @@ -93,9 +95,10 @@ Feature: Project Source Browse Files And I upload a new text file And I fill the upload file commit message And I click on "Upload file" - Then I can see the new text file + Then I can see the new commit message And I am redirected to the fork's new merge request page - And I can see the new commit message + When I click on "Changes" tab + Then I can see the new text file @javascript Scenario: I can replace file and commit @@ -119,9 +122,10 @@ Feature: Project Source Browse Files And I replace it with a text file And I fill the replace file commit message And I click on "Replace file" - Then I can see the new text file - And I am redirected to the fork's new merge request page And I can see the replacement commit message + And I am redirected to the fork's new merge request page + When I click on "Changes" tab + Then I can see the new text file @javascript Scenario: If I enter an illegal file name I see an error message @@ -191,6 +195,7 @@ Feature: Project Source Browse Files And I fill the new branch name And I click on "Commit Changes" Then I am redirected to the new merge request page + Then I click on "Changes" tab And I should see its new content @javascript @wip diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb index bb79424ee08..1cc9e37b075 100644 --- a/features/steps/project/source/browse_files.rb +++ b/features/steps/project/source/browse_files.rb @@ -105,6 +105,10 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps click_button 'Commit Changes' end + step 'I click on "Changes" tab' do + click_link 'Changes' + end + step 'I click on "Create directory"' do click_button 'Create directory' end