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

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'}"
   });