diff options
author | Fatih Acet <acetfatih@gmail.com> | 2016-10-06 16:31:52 +0300 |
---|---|---|
committer | Fatih Acet <acetfatih@gmail.com> | 2016-10-06 16:31:52 +0300 |
commit | d7dd66ed561c48e8b698d750cd4f4ea32379ad30 (patch) | |
tree | 40a584181767be7e3e748c8b1a5eec90abfbc50f /app | |
parent | 0a7678b58ad6f17e78086c2e320647c3a4dd461f (diff) | |
parent | ca3c0c6cd915d44ec2d409b04ab05d964bd5a403 (diff) |
Merge branch 'new-merge-requests-commit-tab-active' into 'master'
MergeRequest new form load diff asynchronously
## What does this MR do?
Don't return diff tab content from the server when request for a new merge request
## Are there points in the code the reviewer needs to double check?
## Why was this MR needed?
To avoid 502 errors due to this controller action takes to much time
## What are the relevant issue numbers?
Relates #13912
## Screenshots (if relevant)
## Does this MR meet the acceptance criteria?
- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- ~~[ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- ~~[ ] API support added~~
- Tests
- [x] Added for this feature/bug
- [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !5844
Diffstat (limited to 'app')
-rw-r--r-- | app/assets/javascripts/lib/utils/common_utils.js | 5 | ||||
-rw-r--r-- | app/assets/javascripts/merge_request_tabs.js | 23 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 66 | ||||
-rw-r--r-- | app/models/merge_request.rb | 4 | ||||
-rw-r--r-- | app/views/projects/diffs/_diffs.html.haml | 3 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_new_diffs.html.haml | 1 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_new_submit.html.haml | 28 |
7 files changed, 84 insertions, 46 deletions
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! - - build_merge_request - @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 - @diffs = @merge_request.diffs(diff_options) if @merge_request.compare - @diff_notes_disabled = true - @pipeline = @merge_request.pipeline - @statuses = @pipeline.statuses.relevant if @pipeline + define_new_vars + end - @note_counts = Note.where(commit_id: @commits.map(&:id)). - group(:commit_id).count + 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 + + 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'}" }); |